This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/qpid-proton.git
commit 3794f2d2bdd64b59c6b1989cd3a00898a84fcc99 Author: Andrew Stitcher <[email protected]> AuthorDate: Fri Sep 18 16:20:52 2026 -0400 PROTON-2977: Enforce the 32 octet maximum on outgoing delivery tags The spec allows a delivery-tag of at most 32 octets, but nothing enforced it. pn_delivery() duplicated whatever the caller passed and the encoder wrote it out. Truncate rather than reject. Rejecting would mean pn_delivery() returning NULL for a reason no existing caller distinguishes, and the tags that overrun come from application code we don't control. Truncate in pn_delivery() rather than when encoding the performative, so that pn_delivery_tag() reports what actually goes on the wire. Truncating only on the way out would leave sender and receiver permanently disagreeing about a delivery's identity, breaking anything that correlates by tag - the unsettled map used for link resume in particular. Outgoing tags only. A receiver's tag comes off the wire, and an over long one there is the peer's violation to report rather than something to rewrite silently underneath our own application. The major potential issue is a tag scheme that puts its distinguishing part last, "producer-instance-7-000001" and the like will stop having unique tags. So warn only once per link, quoting the tag actually sent so it can be matched against a wire trace: Warning per delivery would flood the log of a busy sender. Assisted-By: Claude Opus 5 <[email protected]> --- c/src/core/engine-internal.h | 1 + c/src/core/engine.c | 23 +++++++++ c/src/core/framing.h | 1 + c/tests/engine_test.cpp | 95 +++++++++++++++++++++++++++++++++++++ python/tests/proton_tests/engine.py | 2 +- 5 files changed, 121 insertions(+), 1 deletion(-) diff --git a/c/src/core/engine-internal.h b/c/src/core/engine-internal.h index 16ddd591a..1ddeea913 100644 --- a/c/src/core/engine-internal.h +++ b/c/src/core/engine-internal.h @@ -336,6 +336,7 @@ struct pn_link_t { bool drain; bool detached; bool more_pending; + bool tag_truncated; // a delivery-tag on this link has been truncated (warn once) }; typedef enum pn_disposition_type_t { diff --git a/c/src/core/engine.c b/c/src/core/engine.c index 6e1ea6eeb..1c6f7a9dc 100644 --- a/c/src/core/engine.c +++ b/c/src/core/engine.c @@ -1314,6 +1314,7 @@ pn_link_t *pn_link_new(int type, pn_session_t *session, pn_string_t *name) link->remote_rcv_settle_mode = PN_RCV_FIRST; link->detached = false; link->more_pending = false; + link->tag_truncated = false; link->properties = 0; link->properties_raw = (pn_bytes_t){0, NULL}; link->remote_properties = 0; @@ -1734,6 +1735,28 @@ pn_delivery_t *pn_delivery(pn_link_t *link, pn_delivery_tag_t tag) } delivery->link = link; pn_incref(delivery->link); // keep link until finalized + // The spec allows at most 32 octets of delivery-tag, so truncate rather than put + // an illegal transfer on the wire. Only outbound tags: a receiver's tag comes off + // the wire, and an over long one there is the peer's violation to report, not + // something to silently rewrite under our own application. Warn once per link, as + // an over long tag is a property of the caller's tag scheme, not of any one delivery. + if (tag.size > AMQP_MAX_DELIVERY_TAG_SIZE && pn_link_is_sender(link)) { + size_t original_size = tag.size; + tag.size = AMQP_MAX_DELIVERY_TAG_SIZE; + if (!link->tag_truncated) { + link->tag_truncated = true; + // An unbound connection has no transport, and so no logger of its own. + pn_transport_t *transport = link->session->connection->transport; + const char *name = pn_link_name(link); + char quoted[4*AMQP_MAX_DELIVERY_TAG_SIZE + 1]; // worst case every octet escaped as \xNN + pn_quote_data(quoted, sizeof(quoted), tag.start, tag.size); + PN_LOG(transport ? &transport->logger : pn_default_logger(), + PN_SUBSYSTEM_AMQP, PN_LEVEL_WARNING, + "link '%s': %zu octet delivery-tag truncated to the %d octet maximum, sending '%s'; " + "tags must remain unique amongst the unsettled deliveries on a link", + name ? name : "", original_size, AMQP_MAX_DELIVERY_TAG_SIZE, quoted); + } + } delivery->tag = pn_bytes_dup(tag); pn_disposition_clear(&delivery->local); pn_disposition_clear(&delivery->remote); diff --git a/c/src/core/framing.h b/c/src/core/framing.h index 1c44ed49e..ecc724a7c 100644 --- a/c/src/core/framing.h +++ b/c/src/core/framing.h @@ -33,6 +33,7 @@ #define AMQP_HEADER_SIZE (8) #define AMQP_MIN_MAX_FRAME_SIZE ((uint32_t)512) // minimum allowable max-frame #define AMQP_MAX_WINDOW_SIZE (2147483647) +#define AMQP_MAX_DELIVERY_TAG_SIZE (32) // maximum delivery-tag octets allowed by the spec #define AMQP_FRAME_TYPE (0) #define SASL_FRAME_TYPE (1) diff --git a/c/tests/engine_test.cpp b/c/tests/engine_test.cpp index 2a4825545..60f564aa5 100644 --- a/c/tests/engine_test.cpp +++ b/c/tests/engine_test.cpp @@ -656,3 +656,98 @@ TEST_CASE("max_frame") { pn_transport_free(t2); pn_connection_free(c2); } + +TEST_CASE("delivery_tag_limit") { + // The spec allows 32 octets of delivery-tag. A stringified UUID is 36, a + // common way for applications to overrun the limit. + const char uuid_tag[] = "f81d4fae-7dec-11d0-a765-00a0c91e6bf6"; + REQUIRE(strlen(uuid_tag) == 36); + + pn_connection_t *c1 = pn_connection(); + pn_transport_t *t1 = pn_transport(); + pn_transport_bind(t1, c1); + + pn_connection_t *c2 = pn_connection(); + pn_transport_t *t2 = pn_transport(); + pn_transport_set_server(t2); + pn_transport_bind(t2, c2); + + test_setup(c1, t1, c2, t2); + + pn_link_t *tx = pn_link_head(c1, (PN_LOCAL_ACTIVE | PN_REMOTE_ACTIVE)); + REQUIRE(tx); + pn_link_t *rx = pn_link_head(c2, (PN_LOCAL_ACTIVE | PN_REMOTE_ACTIVE)); + REQUIRE(rx); + pn_link_flow(rx, 10); + + // An over long outgoing tag is truncated to the maximum, keeping the leading octets. + pn_delivery_t *d1 = pn_delivery(tx, pn_dtag(uuid_tag, 36)); + pn_delivery_tag_t sent = pn_delivery_tag(d1); + REQUIRE(sent.size == 32); + REQUIRE(memcmp(sent.start, uuid_tag, 32) == 0); + + while (pump(t1, t2)) { + process_endpoints(c1); + process_endpoints(c2); + } + REQUIRE(pn_delivery_writable(d1)); + pn_link_send(tx, "ABC", 4); + pn_link_advance(tx); + while (pump(t1, t2)) { + process_endpoints(c1); + process_endpoints(c2); + } + + // The receiver sees the same truncated tag, i.e. what we report locally is what + // actually went on the wire. + pn_delivery_t *rd = pn_link_current(rx); + REQUIRE(rd); + pn_delivery_tag_t received = pn_delivery_tag(rd); + REQUIRE(received.size == 32); + REQUIRE(memcmp(received.start, uuid_tag, 32) == 0); + pn_delivery_settle(rd); + pn_delivery_settle(d1); + + // Truncation still applies once the one-shot warning for this link has fired. + pn_delivery_t *d2 = pn_delivery(tx, pn_dtag(uuid_tag, 36)); + REQUIRE(pn_delivery_tag(d2).size == 32); + pn_link_advance(tx); + pn_delivery_settle(d2); + + // A tag exactly at the limit is left alone. + pn_delivery_t *d3 = pn_delivery(tx, pn_dtag(uuid_tag, 32)); + REQUIRE(pn_delivery_tag(d3).size == 32); + pn_link_advance(tx); + pn_delivery_settle(d3); + + // As is a short one. + pn_delivery_t *d4 = pn_delivery(tx, pn_dtag("tag-4", 6)); + REQUIRE(pn_delivery_tag(d4).size == 6); + pn_link_advance(tx); + pn_delivery_settle(d4); + + // Inbound tags are left alone: an over long tag from a peer is its spec violation + // to report, not something to rewrite under the application. + pn_delivery_t *d5 = pn_delivery(rx, pn_dtag(uuid_tag, 36)); + REQUIRE(pn_delivery_tag(d5).size == 36); + + // Binary tags truncate too. A fresh link so the one-shot warning fires again, and + // octets 0x00-0x1f are all unprintable, so the quoted form used in that warning is + // at its longest: 32 * strlen("\\xNN"). + char binary_tag[40]; + for (size_t i = 0; i < sizeof(binary_tag); i++) binary_tag[i] = (char)i; + pn_link_t *tx2 = pn_sender(pn_session_head(c1, 0), "binary-tag-sender"); + REQUIRE(tx2); + pn_delivery_t *d6 = pn_delivery(tx2, pn_dtag(binary_tag, sizeof(binary_tag))); + pn_delivery_tag_t binary_sent = pn_delivery_tag(d6); + REQUIRE(binary_sent.size == 32); + REQUIRE(memcmp(binary_sent.start, binary_tag, 32) == 0); + + pn_transport_unbind(t1); + pn_transport_free(t1); + pn_connection_free(c1); + + pn_transport_unbind(t2); + pn_transport_free(t2); + pn_connection_free(c2); +} diff --git a/python/tests/proton_tests/engine.py b/python/tests/proton_tests/engine.py index b2304c5ec..7fa8aab7c 100644 --- a/python/tests/proton_tests/engine.py +++ b/python/tests/proton_tests/engine.py @@ -1108,7 +1108,7 @@ class TransferTest(Test): (bytearray([1, 2, 32, 254, 255]), '\x01\x02 \udcfe\udcff'), (b'\xff'+(29*b' ')+b'\xff\x00', '\udcff \udcff\x00'), (chr(1024), chr(1024)), - (chr(1024) * 32, chr(1024) * 32) # I think this should fail but it doesn't + (chr(1024) * 16, chr(1024) * 16) # This a bit weird, but should max out the allowed size ] self.rcv.flow(len(test_tags)) --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
