[QPID-3445] Assertion, and unexpected exception in qpid::messaging::decode

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.10, Future
• Fix Version/s: 0.13
• Component/s: C++ Client
• Assignee: Gordon Sim
• Reporter: Paul Colby
• Created: 22/Aug/11 11:22
• Updated: 20/Feb/12 10:15
• Resolved: 24/Aug/11 14:43

Description

Although this is technically two different bug reports, they are very closely related, and should probably be tested / fixed together, so I'm reporting them both here... hope that's okay

Both qpid::messaging::decode functions can assert, or throw an unexpected qpid::framing::IllegalArgumentException on invalid input.

Consider the following code fragment:

const qpid::messaging::Message message("foo");
try {
    qpid::types::Variant::Map map;
    qpid::messaging::decode(message, map); // asserts in qpid::framing::Buffer::getLong
} catch (const qpid::messaging::EncodingException &ex) {
    std::cout << "qpid::messaging::EncodingException " << ex.what() << std::endl;
}
std::cout << "done" << std::endl; // never reached.

In that example, the qpid::messaging::decode function will result in an assertion in qpid::framing::Buffer::getLong as that function assumes / requires the buffer to be at least 4 bytes. Clearly in this case the decode should fail, but ideally it should fail in a catchable way, not an assertion.

I would think the right solution would be to add a minimum size check to the qpid::framing::FieldTable::decode function. But it could also be solved by adding the size check to the qpid::messaging::decode and/or qpid::framing::Buffer::getLong functions.

As a temporary workaround, client code can add a size check before the decode call, like:

const qpid::messaging::Message message("foo");
try {
    if (message.getContent().size() < 4)
        throw qpid::messaging::EncodingException("message too small");
    qpid::types::Variant::Map map;
    qpid::messaging::decode(message, map);
} catch (const qpid::messaging::EncodingException &ex) {
    std::cout << "qpid::messaging::EncodingException " << ex.what() << std::endl;
}
std::cout << "done" << std::endl;

But now if we extend the message a little, so that it is at least 4 bytes long like so:

const qpid::messaging::Message message("\0\0\0\7foo", 7);
try {
    if (message.getContent().size() < 4)
        throw qpid::messaging::EncodingException("message too small");
    qpid::types::Variant::Map map;
    qpid::messaging::decode(message, map);
} catch (const qpid::messaging::EncodingException &ex) {
    std::cout << "qpid::messaging::EncodingException " << ex.what() << std::endl;
}
std::cout << "done" << std::endl; // never reached.

Then we run into a second problem. In that case, the "done" line is still not reached, because a qpid::framing::IllegalArgumentException is thrown in qpid::framing::FieldTable::decode with message "Not enough data for field table.". However, this exception type is not listed in the documentation for the qpid::messaging::decode function - the documentation only mentions EncodingException, and the two share no common ancestry until right back at std::exception.

Although one solution might be just to add IllegalArgumentException to the documentation, I suspect a preferable solution would be to catch the IllegalArgumentException in qpid::messaging::decode and re-throw it as an EncodingException like:

     static void decode(const Message& message, typename C::ObjectType& object, const std::string& encoding)
     {
         checkEncoding(message, encoding);
-        C::decode(message.getContent(), object);
+        try {
+            C::decode(message.getContent(), object);
+        } catch (const qpid::Exception &ex) {
+            throw EncodingException(ex.what());
+        }
     }

A quick code review shows that qpid::framing::FieldTable::decode (and thus qpid::messaging::decode) can also throw the OutOfBounds exception, which, like IllegalArgumentException, descends from qpid::Exception. So a final solution might look something like:

Index: framing/FieldTable.cpp
===================================================================
--- framing/FieldTable.cpp      (revision 1160172)
+++ framing/FieldTable.cpp      (working copy)
@@ -198,10 +198,12 @@

 void FieldTable::decode(Buffer& buffer){
     clear();
+    if (buffer.available() < 4)
+        throw IllegalArgumentException(QPID_MSG("Not enough data for field table."));
     uint32_t len = buffer.getLong();
     if (len) {
         uint32_t available = buffer.available();
-        if (available < len)
+        if ((available < len) || (available < 4))
             throw IllegalArgumentException(QPID_MSG("Not enough data for field table."));
         uint32_t count = buffer.getLong();
         uint32_t leftover = available - len;
Index: messaging/Message.cpp
===================================================================
--- messaging/Message.cpp       (revision 1160172)
+++ messaging/Message.cpp       (working copy)
@@ -21,6 +21,7 @@
 #include "qpid/messaging/Message.h"
 #include "qpid/messaging/MessageImpl.h"
 #include "qpid/amqp_0_10/Codecs.h"
+#include <qpid/Exception.h>
 #include <boost/format.hpp>

 namespace qpid {
@@ -115,7 +116,11 @@
     static void decode(const Message& message, typename C::ObjectType& object, const std::string& encoding)
     {
         checkEncoding(message, encoding);
-        C::decode(message.getContent(), object);
+        try {
+            C::decode(message.getContent(), object);
+        } catch (const qpid::Exception &ex) {
+            throw EncodingException(ex.what());
+        }
     }

     static void encode(const typename C::ObjectType& map, Message& message, const std::string& encoding)

Thoughts?