qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul Colby (JIRA)" <j...@apache.org>
Subject [jira] [Created] (QPID-3445) Assertion, and unexpected exception in qpid::messaging::decode
Date Mon, 22 Aug 2011 11:23:29 GMT
Assertion, and unexpected exception in qpid::messaging::decode
--------------------------------------------------------------

                 Key: QPID-3445
                 URL: https://issues.apache.org/jira/browse/QPID-3445
             Project: Qpid
          Issue Type: Bug
          Components: C++ Client
    Affects Versions: 0.10, Future
            Reporter: Paul Colby


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:
{code}
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.
{code}

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:

{code}
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;
{code}

But now if we extend the message a little, so that it is at least 4 bytes long like so:
{code}
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.
{code}

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:
{code}
     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());
+        }
     }
{code}

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:
{code}
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)
{code}

Thoughts?

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org


Mime
View raw message