Qpid
  1. Qpid
  2. QPID-3445

Assertion, and unexpected exception in qpid::messaging::decode

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.10, Future
    • Fix Version/s: 0.13
    • Component/s: C++ Client
    • Labels:
      None

      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?

        Activity

        Hide
        Paul Colby added a comment -

        Confirmed fixed in 0.14

        Show
        Paul Colby added a comment - Confirmed fixed in 0.14
        Hide
        Paul Colby added a comment -

        Attached a patch including the suggestions above, and equivalent changes to qpid::framing::List::decode too.

        Patch is against http://svn.apache.org/repos/asf/qpid/trunk at revision 1160478.

        Show
        Paul Colby added a comment - Attached a patch including the suggestions above, and equivalent changes to qpid::framing::List::decode too. Patch is against http://svn.apache.org/repos/asf/qpid/trunk at revision 1160478.
        Hide
        Gordon Sim added a comment -

        Looks good to me. Can you attach that patch as a file (with 'grant to ASF' checked)?

        Show
        Gordon Sim added a comment - Looks good to me. Can you attach that patch as a file (with 'grant to ASF' checked)?

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Paul Colby
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development