You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Paul Colby (Closed) (JIRA)" <ji...@apache.org> on 2012/02/20 11:15:38 UTC
[jira] [Closed] (QPID-3445) Assertion, and unexpected exception in
qpid::messaging::decode
[ 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-subscribe@qpid.apache.org