This is an automated email from the ASF dual-hosted git repository.
astitcher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/qpid-proton.git
The following commit(s) were added to refs/heads/main by this push:
new 40cb24b PROTON-2535: Parse and generate handle_max in the begin frame
40cb24b is described below
commit 40cb24b5ffee5e2e4d08453329a0e52bf222963d
Author: Andrew Stitcher <[email protected]>
AuthorDate: Fri Mar 18 18:06:27 2022 -0400
PROTON-2535: Parse and generate handle_max in the begin frame
We want to put handle_max into the frame to avoid our peer from sending
a handle that is big enough to seem negative - we use negative handles
for special purposes.
Equally, we should respect any handle_max our peer sends us.
[Bug found by the fuzzer]
---
c/src/core/engine-internal.h | 3 ++
c/src/core/engine.c | 2 ++
c/src/core/transport.c | 33 ++++++++++++++++-----
.../crash/leak-5289430926098432-1 | Bin 0 -> 121 bytes
c/tools/codec-generator/specs.json | 4 +--
5 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/c/src/core/engine-internal.h b/c/src/core/engine-internal.h
index 09ff3ec..7554dd3 100644
--- a/c/src/core/engine-internal.h
+++ b/c/src/core/engine-internal.h
@@ -94,6 +94,8 @@ typedef struct {
pn_sequence_t disp_first;
pn_sequence_t disp_last;
// XXX: stop using negative numbers
+ #define PN_IMPL_HANDLE_MAX 0x7fffffff
+ uint32_t remote_handle_max;
uint16_t local_channel;
uint16_t remote_channel;
bool incoming_init;
@@ -259,6 +261,7 @@ struct pn_session_t {
pn_list_t *freed;
pn_record_t *context;
size_t incoming_capacity;
+ uint32_t local_handle_max;
pn_sequence_t incoming_bytes;
pn_sequence_t outgoing_bytes;
pn_sequence_t incoming_deliveries;
diff --git a/c/src/core/engine.c b/c/src/core/engine.c
index 1aa1992..d16cf65 100644
--- a/c/src/core/engine.c
+++ b/c/src/core/engine.c
@@ -1005,9 +1005,11 @@ pn_session_t *pn_session(pn_connection_t *conn)
ssn->incoming_deliveries = 0;
ssn->outgoing_deliveries = 0;
ssn->outgoing_window = AMQP_MAX_WINDOW_SIZE;
+ ssn->local_handle_max = PN_IMPL_HANDLE_MAX;
// begin transport state
memset(&ssn->state, 0, sizeof(ssn->state));
+ ssn->state.remote_handle_max = UINT32_MAX;
ssn->state.local_channel = (uint16_t)-1;
ssn->state.remote_channel = (uint16_t)-1;
pn_delivery_map_init(&ssn->state.incoming, 0);
diff --git a/c/src/core/transport.c b/c/src/core/transport.c
index e669a06..b05f25a 100644
--- a/c/src/core/transport.c
+++ b/c/src/core/transport.c
@@ -1101,8 +1101,12 @@ int pn_do_begin(pn_transport_t *transport, uint8_t
frame_type, uint16_t channel,
bool reply;
uint16_t remote_channel;
pn_sequence_t next;
+ uint32_t incoming_window;
+ uint32_t outgoing_window;
+ bool handle_max_q;
+ uint32_t handle_max;
- pn_amqp_decode_DqEQHIe(payload, &reply, &remote_channel, &next);
+ pn_amqp_decode_DqEQHIIIQIe(payload, &reply, &remote_channel, &next,
&incoming_window, &outgoing_window, &handle_max_q, &handle_max);
// AMQP 1.0 section 2.7.1 - if the peer doesn't honor our channel_max --
// express our displeasure by closing the connection with a framing error.
@@ -1131,6 +1135,9 @@ int pn_do_begin(pn_transport_t *transport, uint8_t
frame_type, uint16_t channel,
ssn = pn_session(transport->connection);
}
ssn->state.incoming_transfer_count = next;
+ if (handle_max_q) {
+ ssn->state.remote_handle_max = handle_max;
+ }
pni_map_remote_channel(ssn, channel);
PN_SET_REMOTE(ssn->endpoint.state, PN_REMOTE_ACTIVE);
pn_collector_put(transport->connection->collector, PN_OBJECT, ssn,
PN_SESSION_REMOTE_OPEN);
@@ -1242,6 +1249,13 @@ int pn_do_attach(pn_transport_t *transport, uint8_t
frame_type, uint16_t channel
pn_free(rem_props);
return PN_EOS;
}
+ if (handle > ssn->local_handle_max) {
+ pn_do_error(transport, "amqp:connection:framing-error",
+ "remote handle %u is above handle_max %u", handle,
ssn->local_handle_max);
+ if (strheap) free(strheap);
+ pn_free(rem_props);
+ return PN_EOS;
+ }
pn_link_t *link = pni_find_link(ssn, name, is_sender);
if (link && (int32_t)link->state.remote_handle >= 0) {
pn_do_error(transport, "amqp:invalid-field", "link name already attached:
%s", strname);
@@ -1944,12 +1958,13 @@ static int pni_process_ssn_setup(pn_transport_t
*transport, pn_endpoint_t *endpo
}
state->incoming_window = pni_session_incoming_window(ssn);
state->outgoing_window = pni_session_outgoing_window(ssn);
- /* "DL[?HIII]" */
- pn_bytes_t buf = pn_amqp_encode_DLEQHIIIe(transport->frame, BEGIN,
+ /* "DL[?HIIII]" */
+ pn_bytes_t buf = pn_amqp_encode_DLEQHIIIIe(transport->frame, BEGIN,
((int16_t) state->remote_channel >= 0),
state->remote_channel,
state->outgoing_transfer_count,
state->incoming_window,
- state->outgoing_window);
+ state->outgoing_window,
+ ssn->local_handle_max);
pn_framing_send_amqp(transport, state->local_channel, buf);
}
}
@@ -1979,9 +1994,8 @@ static int pni_map_local_handle(pn_link_t *link) {
pn_link_state_t *state = &link->state;
pn_session_state_t *ssn_state = &link->session->state;
int valid;
- // XXX TODO MICK: once changes are made to handle_max, change this hardcoded
value to something reasonable.
- state->local_handle = allocate_alias(ssn_state->local_handles, 65536, &
valid);
- if ( ! valid )
+ state->local_handle = allocate_alias(ssn_state->local_handles,
ssn_state->remote_handle_max, &valid);
+ if ( !valid )
return 0;
pn_hash_put(ssn_state->local_handles, state->local_handle, link);
pn_ep_incref(&link->endpoint);
@@ -1999,7 +2013,10 @@ static int pni_process_link_setup(pn_transport_t
*transport, pn_endpoint_t *endp
if (((int16_t) ssn_state->local_channel >= 0) &&
!(endpoint->state & PN_LOCAL_UNINIT) && state->local_handle ==
(uint32_t) -1)
{
- pni_map_local_handle(link);
+ if (! pni_map_local_handle(link)) {
+ pn_logger_logf(&transport->logger, PN_SUBSYSTEM_AMQP,
PN_LEVEL_WARNING, "unable to find an open available handle within limit of %d",
ssn_state->remote_handle_max );
+ return PN_ERR;
+ }
const pn_distribution_mode_t dist_mode = (pn_distribution_mode_t)
link->source.distribution_mode;
if (link->target.type == PN_COORDINATOR) {
/* "DL[SIoBB?DL[SIsIoC?sCnCC]DL[C]nnI]" */
diff --git a/c/tests/fuzz/fuzz-connection-driver/crash/leak-5289430926098432-1
b/c/tests/fuzz/fuzz-connection-driver/crash/leak-5289430926098432-1
new file mode 100644
index 0000000..04ee2d4
Binary files /dev/null and
b/c/tests/fuzz/fuzz-connection-driver/crash/leak-5289430926098432-1 differ
diff --git a/c/tools/codec-generator/specs.json
b/c/tools/codec-generator/specs.json
index 15bdcdb..2775073 100644
--- a/c/tools/codec-generator/specs.json
+++ b/c/tools/codec-generator/specs.json
@@ -2,7 +2,7 @@
"fill_specs": [
"DLC",
"DL[?DL[sSC]]",
- "DL[?HIII]",
+ "DL[?HIIII]",
"DL[?IIII?I?I?In?o]",
"DL[?o?B?I?o?I]",
"DL[@T[*s]]",
@@ -25,7 +25,7 @@
"D.[.....D..D.[C]...]",
"D.[.....D..DL....]",
"D.[.....D.[.....C.C.CC]]",
- "D.[?HI]",
+ "D.[?HIII?I]",
"D.[?IIII?I?II.o]",
"D.[?S?S?I?HI..CCC]",
"D.[I?Iz.?oo.D?LRooo]",
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
