[ https://issues.apache.org/jira/browse/QPID-3445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul Colby closed QPID-3445. ---------------------------- Confirmed fixed in 0.14 :) > 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 > Assignee: Gordon Sim > Fix For: 0.13 > > Attachments: QPID-3445.diff > > > 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. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa 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