You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by as...@apache.org on 2018/07/30 16:34:58 UTC
[4/8] qpid-proton git commit: PROTON-1903: Bug found by OSS Fuzz
project - Had to fix pn_sequnce_t to be unsigned to allow it to wrap on
increment as only unsigned int has defined behaviour on overflow. signed DOES
NOT. - Python and Ruby tests fixed to
PROTON-1903: Bug found by OSS Fuzz project
- Had to fix pn_sequnce_t to be unsigned to allow it to wrap on increment
as only unsigned int has defined behaviour on overflow. signed DOES NOT.
- Python and Ruby tests fixed to not try negative sequence-no values as the
AMQP spec explicitly defines this to be unsigned.
OSS-Fuzz bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8308
Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/2f436803
Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/2f436803
Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/2f436803
Branch: refs/heads/master
Commit: 2f43680313c761d525354bb64fa4e5be79a51abb
Parents: 6dfcf32
Author: Andrew Stitcher <as...@apache.org>
Authored: Sun Jul 29 00:01:28 2018 -0400
Committer: Andrew Stitcher <as...@apache.org>
Committed: Mon Jul 30 11:39:15 2018 -0400
----------------------------------------------------------------------
c/include/proton/types.h | 2 +-
c/src/core/transport.c | 15 ++++++---------
c/src/messenger/store.c | 6 +++---
.../fuzz-connection-driver/crash/5938290925502464 | Bin 0 -> 147 bytes
python/tests/proton_tests/message.py | 2 +-
ruby/spec/message_spec.rb | 8 +-------
6 files changed, 12 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/c/include/proton/types.h
----------------------------------------------------------------------
diff --git a/c/include/proton/types.h b/c/include/proton/types.h
index 9df0cc6..2619b5c 100644
--- a/c/include/proton/types.h
+++ b/c/include/proton/types.h
@@ -135,7 +135,7 @@ extern "C" {
*
* @ingroup api_types
*/
-typedef int32_t pn_sequence_t;
+typedef uint32_t pn_sequence_t;
/**
* A span of time in milliseconds.
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/c/src/core/transport.c
----------------------------------------------------------------------
diff --git a/c/src/core/transport.c b/c/src/core/transport.c
index 9c448d0..b47f680 100644
--- a/c/src/core/transport.c
+++ b/c/src/core/transport.c
@@ -975,7 +975,7 @@ static int pni_post_amqp_transfer_frame(pn_transport_t *transport, uint16_t ch,
bool batchable)
{
bool more_flag = more;
- int framecount = 0;
+ unsigned framecount = 0;
pn_buffer_t *frame = transport->frame;
// create preformatives, assuming 'more' flag need not change
@@ -1626,13 +1626,8 @@ static int pn_scan_error(pn_data_t *data, pn_condition_t *condition, const char
return 0;
}
-/*
- This operator, inspired by code for the qpid cpp broker, gives the correct
- result when comparing sequence numbers implemented in a signed integer type.
-*/
-
-static inline int sequence_cmp(pn_sequence_t a, pn_sequence_t b) {
- return a-b;
+static inline bool sequence_lte(pn_sequence_t a, pn_sequence_t b) {
+ return b-a <= INT32_MAX;
}
int pn_do_disposition(pn_transport_t *transport, uint8_t frame_type, uint16_t channel, pn_data_t *args, const pn_bytes_t *payload)
@@ -1664,7 +1659,9 @@ int pn_do_disposition(pn_transport_t *transport, uint8_t frame_type, uint16_t ch
bool remote_data = (pn_data_next(transport->disp_data) &&
pn_data_get_list(transport->disp_data) > 0);
- for (pn_sequence_t id = first; sequence_cmp(id, last) <= 0; ++id) {
+ // TODO: We need to clamp the first & last values here to the actual first and last unsettled
+ // Otherwise we could just be told to process any old sequence.
+ for (pn_sequence_t id = first; sequence_lte(id, last); ++id) {
pn_delivery_t *delivery = pni_delivery_map_get(deliveries, id);
pn_disposition_t *remote = &delivery->remote;
if (delivery) {
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/c/src/messenger/store.c
----------------------------------------------------------------------
diff --git a/c/src/messenger/store.c b/c/src/messenger/store.c
index 44f24f1..6f6e772 100644
--- a/c/src/messenger/store.c
+++ b/c/src/messenger/store.c
@@ -39,7 +39,7 @@ struct pni_store_t {
pni_entry_t *store_tail;
pn_hash_t *tracked;
size_t size;
- int window;
+ unsigned window;
pn_sequence_t lwm;
pn_sequence_t hwm;
};
@@ -348,7 +348,7 @@ pni_entry_t *pni_store_entry(pni_store_t *store, pn_sequence_t id)
bool pni_store_tracking(pni_store_t *store, pn_sequence_t id)
{
- return (id - store->lwm >= 0) && (store->hwm - id > 0);
+ return (id - store->lwm < INT32_MAX) && (store->hwm - id < INT32_MAX + 1u);
}
pn_sequence_t pni_entry_track(pni_entry_t *entry)
@@ -359,7 +359,7 @@ pn_sequence_t pni_entry_track(pni_entry_t *entry)
entry->id = store->hwm++;
pn_hash_put(store->tracked, entry->id, entry);
- if (store->window >= 0) {
+ if (store->window < INT32_MAX) {
while (store->hwm - store->lwm > store->window) {
pni_entry_t *e = pni_store_entry(store, store->lwm);
if (e) {
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/c/tests/fuzz/fuzz-connection-driver/crash/5938290925502464
----------------------------------------------------------------------
diff --git a/c/tests/fuzz/fuzz-connection-driver/crash/5938290925502464 b/c/tests/fuzz/fuzz-connection-driver/crash/5938290925502464
new file mode 100644
index 0000000..411b30d
Binary files /dev/null and b/c/tests/fuzz/fuzz-connection-driver/crash/5938290925502464 differ
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/python/tests/proton_tests/message.py
----------------------------------------------------------------------
diff --git a/python/tests/proton_tests/message.py b/python/tests/proton_tests/message.py
index 26a3dd2..cb8c567 100644
--- a/python/tests/proton_tests/message.py
+++ b/python/tests/proton_tests/message.py
@@ -99,7 +99,7 @@ class AccessorsTest(Test):
self._test_str("group_id")
def testGroupSequence(self):
- self._test("group_sequence", 0, (0, -10, 10, 20, -20))
+ self._test("group_sequence", 0, (0, 1, 10, 20, 4294967294, 4294967295))
def testReplyToGroupId(self):
self._test_str("reply_to_group_id")
http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/2f436803/ruby/spec/message_spec.rb
----------------------------------------------------------------------
diff --git a/ruby/spec/message_spec.rb b/ruby/spec/message_spec.rb
index 5383f5a..7976716 100644
--- a/ruby/spec/message_spec.rb
+++ b/ruby/spec/message_spec.rb
@@ -331,19 +331,13 @@ module Qpid
}.must_raise(TypeError)
end
- it "can have a negative group sequence" do
- seq = (0 - rand(32767))
- @message.group_sequence = seq
- @message.group_sequence.must_equal(seq)
- end
-
it "can have a zero group sequence" do
@message.group_sequence = 0
@message.group_sequence.must_equal(0)
end
it "has a group sequence" do
- id = rand(32767)
+ id = rand(4294967295)
@message.group_sequence = id
@message.group_sequence.must_equal(id)
end
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org