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
