From: Jelte Jansen <jelte@isc.org>
Date: Wed, 29 Jun 2011 19:46:05 +0000 (+0200)
Subject: [trac714] handle bad data on the cc session in cfgmgr
X-Git-Tag: perftcpdns_before_epoll~231^2~25^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7e41b9c3e2e1ca809ed4ea6de67c843a1a0d7680;p=thirdparty%2Fkea.git

[trac714] handle bad data on the cc session in cfgmgr
---

diff --git a/src/lib/python/isc/cc/session.py b/src/lib/python/isc/cc/session.py
index fb7dd06ff0..f6b62653be 100644
--- a/src/lib/python/isc/cc/session.py
+++ b/src/lib/python/isc/cc/session.py
@@ -93,6 +93,19 @@ class Session:
                 self._socket.send(msg)
 
     def recvmsg(self, nonblock = True, seq = None):
+        """Reads a message. If nonblock is true, and there is no
+           message to read, it returns (None, None).
+           If seq is not None, it should be a value as returned by
+           group_sendmsg(), in which case only the response to
+           that message is returned, and others will be queued until
+           the next call to this method.
+           If seq is None, only messages that are *not* responses
+           will be returned, and responses will be queued.
+           The queue is checked for relevant messages before data
+           is read from the socket.
+           Raises a SessionError if there is a JSON decode problem in
+           the message that is read, or if the session has been closed
+           prior to the call of recvmsg()"""
         with self._lock:
             if len(self._queue) > 0:
                 i = 0;
@@ -109,16 +122,22 @@ class Session:
             if data and len(data) > 2:
                 header_length = struct.unpack('>H', data[0:2])[0]
                 data_length = len(data) - 2 - header_length
-                if data_length > 0:
-                    env = isc.cc.message.from_wire(data[2:header_length+2])
-                    msg = isc.cc.message.from_wire(data[header_length + 2:])
-                    if (seq == None and "reply" not in env) or (seq != None and "reply" in env and seq == env["reply"]):
-                        return env, msg
+                try:
+                    if data_length > 0:
+                        env = isc.cc.message.from_wire(data[2:header_length+2])
+                        msg = isc.cc.message.from_wire(data[header_length + 2:])
+                        if (seq == None and "reply" not in env) or (seq != None and "reply" in env and seq == env["reply"]):
+                            return env, msg
+                        else:
+                            self._queue.append((env,msg))
+                            return self.recvmsg(nonblock, seq)
                     else:
-                        self._queue.append((env,msg))
-                        return self.recvmsg(nonblock, seq)
-                else:
-                    return isc.cc.message.from_wire(data[2:header_length+2]), None
+                        return isc.cc.message.from_wire(data[2:header_length+2]), None
+                except ValueError as ve:
+                    # TODO: when we have logging here, add a debug
+                    # message printing the data that we were unable
+                    # to parse as JSON
+                    raise SessionError(ve)
             return None, None
 
     def _receive_bytes(self, size):
diff --git a/src/lib/python/isc/cc/tests/session_test.py b/src/lib/python/isc/cc/tests/session_test.py
index fe35a6cbfa..772ed0c961 100644
--- a/src/lib/python/isc/cc/tests/session_test.py
+++ b/src/lib/python/isc/cc/tests/session_test.py
@@ -274,6 +274,16 @@ class testSession(unittest.TestCase):
         self.assertEqual({"hello": "b"}, msg)
         self.assertFalse(sess.has_queued_msgs())
 
+    def test_recv_bad_msg(self):
+        sess = MySession()
+        self.assertFalse(sess.has_queued_msgs())
+        sess._socket.addrecv({'to': 'someone' }, {'hello': 'b'})
+        sess._socket.addrecv({'to': 'someone', 'reply': 1}, {'hello': 'a'})
+        # mangle the bytes a bit
+        sess._socket.recvqueue[5] = sess._socket.recvqueue[5] - 2
+        sess._socket.recvqueue = sess._socket.recvqueue[:-2]
+        self.assertRaises(SessionError, sess.recvmsg, True, 1)
+
     def test_next_sequence(self):
         sess = MySession()
         self.assertEqual(sess._sequence, 1)
diff --git a/src/lib/python/isc/config/cfgmgr.py b/src/lib/python/isc/config/cfgmgr.py
index 83db159794..18e001c306 100644
--- a/src/lib/python/isc/config/cfgmgr.py
+++ b/src/lib/python/isc/config/cfgmgr.py
@@ -380,6 +380,9 @@ class ConfigManager:
                 answer, env = self.cc.group_recvmsg(False, seq)
             except isc.cc.SessionTimeout:
                 answer = ccsession.create_answer(1, "Timeout waiting for answer from " + module_name)
+            except isc.cc.SessionError as se:
+                logger.error(CFGMGR_BAD_UPDATE_RESPONSE_FROM_MODULE, module_name, se)
+                answer = ccsession.create_answer(1, "Unable to parse response from " + module_name + ": " + str(se))
         if answer:
             rcode, val = ccsession.parse_answer(answer)
             if rcode == 0:
diff --git a/src/lib/python/isc/config/cfgmgr_messages.mes b/src/lib/python/isc/config/cfgmgr_messages.mes
index 9355e4d976..eb09ab4692 100644
--- a/src/lib/python/isc/config/cfgmgr_messages.mes
+++ b/src/lib/python/isc/config/cfgmgr_messages.mes
@@ -20,6 +20,12 @@ An older version of the configuration database has been found, from which
 there was an automatic upgrade path to the current version. These changes
 are now applied, and no action from the administrator is necessary.
 
+% CFGMGR_BAD_UPDATE_RESPONSE_FROM_MODULE Unable to parse response from module %1: %2
+The configuration manager sent a configuration update to a module, but
+the module responded with an answer that could not be parsed. The answer
+message appears to be invalid JSON data, or not decodable to a string.
+This is likely to be a problem in the module in question.
+
 % CFGMGR_CC_SESSION_ERROR Error connecting to command channel: %1
 The configuration manager daemon was unable to connect to the messaging
 system. The most likely cause is that msgq is not running.