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-subscr...@qpid.apache.org