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 (JIRA)" <ji...@apache.org> on 2011/08/22 13:23:29 UTC
[jira] [Created] (QPID-3445) Assertion, and unexpected exception in
qpid::messaging::decode
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-subscribe@qpid.apache.org
[jira] [Assigned] (QPID-3445) Assertion, and unexpected exception
in qpid::messaging::decode
Posted by "Gordon Sim (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gordon Sim reassigned QPID-3445:
--------------------------------
Assignee: Gordon Sim
> 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
>
> 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-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3445) Assertion, and unexpected exception in
qpid::messaging::decode
Posted by "Paul Colby (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Paul Colby updated QPID-3445:
-----------------------------
Attachment: QPID-3445.diff
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.
> 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
> 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.
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
[jira] [Closed] (QPID-3445) Assertion, and unexpected exception in
qpid::messaging::decode
Posted by "Paul Colby (Closed) (JIRA)" <ji...@apache.org>.
[ 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
[jira] [Commented] (QPID-3445) Assertion, and unexpected exception
in qpid::messaging::decode
Posted by "Gordon Sim (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13088647#comment-13088647 ]
Gordon Sim commented on QPID-3445:
----------------------------------
Looks good to me. Can you attach that patch as a file (with 'grant to ASF' checked)?
> 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-subscribe@qpid.apache.org
[jira] [Resolved] (QPID-3445) Assertion, and unexpected exception
in qpid::messaging::decode
Posted by "Gordon Sim (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3445?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Gordon Sim resolved QPID-3445.
------------------------------
Resolution: Fixed
Fix Version/s: 0.13
> 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.
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