[2617] make sure FD handling is performed only when it's still valid.

This eliminates the possibility of hitting MSGQ_READ_UNKNOWN_FD, and, in fact,
we can even deprecate this message.   And then, process_socket() becomes
a 2-line function called only by _process_fd() and can be integrated into
the latter.  so it's done, too.

diff --git a/src/bin/msgq/msgq.py.in b/src/bin/msgq/msgq.py.in
index c8ef5d6be3aa26f2a36bf95daaca458296ad3892..935d92736c17508ea39a7b86b3202c28abfc9420 100755 (executable)
--- a/src/bin/msgq/msgq.py.in
+++ b/src/bin/msgq/msgq.py.in
@@ -319,14 +319,6 @@ class MsgQ:
         else:
             self.add_kqueue_socket(newsocket)
 
-    def process_socket(self, fd):
-        """Process a read on a socket."""
-        if not fd in self.sockets:
-            logger.error(MSGQ_READ_UNKNOWN_FD, fd)
-            return
-        sock = self.sockets[fd]
-        self.process_packet(fd, sock)
-
     def kill_socket(self, fd, sock):
         """Fully close down the socket."""
         if self.poller:
@@ -639,11 +631,14 @@ class MsgQ:
         from tests.
 
         '''
-        if writable:
+        # We need to check if FD is still in the sockets dict, because
+        # it's possible that the socket has been "killed" while processing
+        # other FDs; it's even possible it's killed within this method.
+        if writable and fd in self.sockets:
             self.__process_write(fd)
-        if readable:
-            self.process_socket(fd)
-        if closed:
+        if readable and fd in self.sockets:
+            self.process_packet(fd, self.sockets[fd])
+        if closed and fd in self.sockets:
             self.kill_socket(fd, self.sockets[fd])
 
     def stop(self):

diff --git a/src/bin/msgq/msgq_messages.mes b/src/bin/msgq/msgq_messages.mes
index 0b4440585a1d89b6f898da660a9b7618c995c0e7..a5c57ff04dae957576e9ee5d55841368bcf41fdc 100644 (file)
--- a/src/bin/msgq/msgq_messages.mes
+++ b/src/bin/msgq/msgq_messages.mes
@@ -68,11 +68,6 @@ happen and it is either a programmer error or OS bug. The event is ignored. The
 number noted as the event is the raw encoded value, which might be useful to
 the authors when figuring the problem out.
 
-% MSGQ_READ_UNKNOWN_FD Got read on strange socket %1
-The OS reported a file descriptor is ready to read. But the daemon doesn't know
-the mentioned file descriptor, which is either a programmer error or OS bug.
-The read event is ignored.
-
 % MSGQ_RECV_ERR Error reading from socket %1: %2
 There was a low-level error when reading from a socket. The error is logged and
 the corresponding socket is dropped.

diff --git a/src/bin/msgq/tests/msgq_test.py b/src/bin/msgq/tests/msgq_test.py
index 4c2988dac985e6e8f413746c1f4192f971ebdb15..13360d36545ec21d47342db489ffb263463fc515 100644 (file)
--- a/src/bin/msgq/tests/msgq_test.py
+++ b/src/bin/msgq/tests/msgq_test.py
@@ -588,22 +588,26 @@ class SocketTests(unittest.TestCase):
             self.error_called = 0
             self.warn_called = 0
             self.orig_logger = logger
-        def error(self, id, fd_arg, sock_arg):
+        def error(self, *args):
             self.error_called += 1
-            self.orig_logger.error(id, fd_arg, sock_arg)
-        def warn(self, id, fd_arg):
+            self.orig_logger.error(*args)
+        def warn(self, *args):
             self.warn_called += 1
-            self.orig_logger.warn(id, fd_arg)
+            self.orig_logger.warn(*args)
 
     def mock_kill_socket(self, fileno, sock):
         '''A replacement of MsgQ.kill_socket method for inspection.'''
         self.__killed_socket = (fileno, sock)
+        if fileno in self.__msgq.sockets:
+            del self.__msgq.sockets[fileno]
 
     def setUp(self):
         self.__msgq = MsgQ()
         self.__msgq.kill_socket = self.mock_kill_socket
         self.__sock = self.MockSocket()
         self.__data = b'dummy'
+        self.__msgq.sockets[42] = self.__sock
+        self.__msgq.sendbuffs[42] = (None, b'testdata')
         self.__sock_error = socket.error()
         self.__killed_socket = None
         self.__logger = self.LoggerWrapper(msgq.logger)
@@ -657,6 +661,32 @@ class SocketTests(unittest.TestCase):
             self.assertEqual(expected_errors, self.__logger.error_called)
             self.assertEqual(expected_warns, self.__logger.warn_called)
 
+    def test_process_fd_read_after_bad_write(self):
+        '''Check the specific case of write fail followed by read attempt.
+
+        The write failure results in kill_socket, then read shouldn't tried.
+
+        '''
+        self.__sock_error.errno = errno.EPIPE
+        self.__sock.ex_on_send = self.__sock_error
+        self.__msgq.process_socket = None # if called, trigger an exception
+        self.__msgq._process_fd(42, True, True, False) # shouldn't crash
+
+        # check the socket is deleted from the fileno=>sock dictionary
+        self.assertEqual({}, self.__msgq.sockets)
+
+    def test_process_fd_close_after_bad_write(self):
+        '''Similar to the previous, but for checking dup'ed kill attempt'''
+        self.__sock_error.errno = errno.EPIPE
+        self.__sock.ex_on_send = self.__sock_error
+        self.__msgq._process_fd(42, True, False, True) # shouldn't crash
+        self.assertEqual({}, self.__msgq.sockets)
+
+    def test_process_fd_writer_after_close(self):
+        '''Emulate a "writable" socket has been already closed and killed.'''
+        # This just shouldn't crash
+        self.__msgq._process_fd(4200, True, False, False)
+
 if __name__ == '__main__':
     isc.log.resetUnitTestRootLogger()
     unittest.main()