[EventHub][ServiceBus] Fix pyAMQP decode of performatives with omitted trailing null fields#47660
Open
j7nw4r wants to merge 7 commits into
Open
[EventHub][ServiceBus] Fix pyAMQP decode of performatives with omitted trailing null fields#47660j7nw4r wants to merge 7 commits into
j7nw4r wants to merge 7 commits into
Conversation
j7nw4r
marked this pull request as ready for review
July 15, 2026 22:56
j7nw4r
requested review from
EldertGrootenboer,
SwayGom,
axisc,
hmlam,
sagar0207,
sjkwak and
skarri-microsoft
as code owners
July 15, 2026 22:56
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes pyAMQP decoding of performatives with omitted trailing fields across Event Hubs and Service Bus.
Changes:
- Pads shortened performatives and supports
list0. - Adds regression tests for Open and Transfer frames.
- Updates changelogs and Event Hubs versioning.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/servicebus/azure-servicebus/tests/unittests/test_pyamqp_decode.py |
Adds decoder regression tests. |
sdk/servicebus/azure-servicebus/CHANGELOG.md |
Documents the fix. |
sdk/servicebus/azure-servicebus/azure/servicebus/_pyamqp/_decode.py |
Implements performative padding and list0 decoding. |
sdk/eventhub/azure-eventhub/tests/pyamqp_tests/unittest/test_decode.py |
Adds equivalent regression tests. |
sdk/eventhub/azure-eventhub/CHANGELOG.md |
Adds the 5.15.2 release entry. |
sdk/eventhub/azure-eventhub/azure/eventhub/_version.py |
Bumps version to 5.15.2. |
sdk/eventhub/azure-eventhub/azure/eventhub/_pyamqp/_decode.py |
Mirrors the decoder fix. |
j7nw4r
force-pushed
the
fix/pyamqp-decode-short-list
branch
from
July 16, 2026 18:19
6fecb8a to
f89b410
Compare
AMQP 1.0 section 1.4 lets a sender omit trailing null fields, so an incoming performative described-list can be shorter than the full field count. The pyAMQP decoder built the field list from the wire count, and consumers then accessed fixed indices (frame[9], OpenFrame(*frame)), raising IndexError/TypeError on a short list. decode_frame now pads the decoded list up to the performative's full field count, so omitted trailing fields read back as None. Applied to both the Event Hubs and Service Bus vendored copies, with regression tests and changelog entries.
…ives Add the # type: ignore # pylint: disable=protected-access suppressions on the _PERFORMATIVE_FIELD_COUNT comprehension, matching every other access to the protected _code/_definition attributes in this engine (see _encode.py), so the pylint and mypy CI gates stay green. Also handle the AMQP list0 (0x45) body encoding in decode_frame: a performative whose fields are all omitted may arrive as list0, which carries no count byte. Previously this read data[5] out of range and raised IndexError before the padding ran. It is now treated as zero fields and padded to the full field count, closing the "omitted trailing null fields" case for End and Close. Add list0 regression tests to both the eventhub and servicebus suites.
…drift Extend the decode regression tests for the trailing-null-omission fix: - Parametrized short-list8 decode over Begin, Attach, Disposition, and Detach, the no-default namedtuples that raised TypeError on a short unpack pre-fix. Only Open was previously covered. - Short performative encoded as a list32 (0xd0) body, exercising the count/offset branch that no existing test reached (all fixtures used list8). - Short SASLInit and SASLOutcome frames, which also have required fields. - A skip-guarded test asserting the eventhub and servicebus _pyamqp copies of _decode.py, _encode.py, and performatives.py stay byte-identical, so a fix applied to one copy but not the other is caught. It skips when the sibling package source is not present (the packages ship separately). Applied identically to both the eventhub and servicebus decode test suites.
Decoding a short performative padded every omitted trailing field with None. For a field whose AMQP-defined default is non-null this is wrong: a minimal Open frame legitimately omits max_frame_size (default 4294967295), and _connection._incoming_open reads it positionally and numerically (`frame[2] < 512`), which raises TypeError on None. Pad each omitted field with its field default from the performative _definition instead of None, via a _PERFORMATIVE_FIELD_DEFAULTS map; _PERFORMATIVE_FIELD_COUNT is now derived from it so the two stay in lockstep. Applied to both the Event Hubs and Service Bus copies of the vendored _pyamqp engine. Tests exercise the incoming-Open numeric path (max_frame_size/channel_max materialize to their defaults and the `< 512` comparison does not raise), and the no-default-performative and transfer tests now assert the padded tail equals each field's spec default (e.g. Disposition.batchable is False, Transfer.message_format is 0).
The _PERFORMATIVE_FIELD_DEFAULTS comprehension reads _code and _definition off the performative classes directly. Those attributes are assigned at import time in performatives.py, so pylint 4.0.4 with azure-pylint-guidelines-checker cannot resolve them statically and raised 28 E1101(no-member) errors, failing the Analyze job with exit 2. Extend the existing protected-access disable on that line to also cover no-member. Comment-only, behavior unchanged; applied to both byte-identical _pyamqp copies.
j7nw4r
force-pushed
the
fix/pyamqp-decode-short-list
branch
from
July 16, 2026 21:10
fbd200d to
50c8857
Compare
The api-consistency workflow requires a committed api.md and api.metadata.yml for every package a PR touches. azure-eventhub had neither, so the check reported "Missing required API files" once this branch touched the package. Generate both with azpysdk apistub (apiview-stub-generator 0.3.28, the version eng/apiview_reqs.txt pins), which the workflow diff-gates against a fresh regeneration. The change touches only the private _pyamqp engine, so the public API surface is unchanged; this is the package's existing public API captured for the first time. Verified the local toolchain reproduces the committed azure-servicebus api.md byte-for-byte (matching apiMdSha256), so the generated eventhub file matches what CI regenerates.
…ault Only trailing fields of a performative may be omitted, so a sender that sets a later field while wanting an earlier one's default must encode that earlier field as an explicit null. A decoded null for a field whose AMQP default is non-null therefore also means that default. Decoding now normalizes those nulls so an explicit null reads back identically to an omitted field: an Open that nulls max_frame_size comes back as 4294967295 rather than None, and _connection._incoming_open's frame[2] < 512 no longer raises TypeError. Fields whose declared default is null are left as None. Applied to both byte-identical _pyamqp copies with a regression test that nulls max_frame_size while setting channel_max after it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pyAMQP transport crashes when decoding an AMQP performative whose trailing null fields were omitted by the sender. The decoded performative list is now padded to its full field count so positional field access and namedtuple unpacking remain safe, with omitted trailing fields reading back as null.
Motivation
AMQP 1.0 section 1.4 permits a sender to omit the trailing null fields of a described list, so an incoming performative can arrive shorter than its full field count. The decoder in
_decode.pybuilt the field list from the element count on the wire, and downstream consumers then read fixed positions (for exampleframe[9]for Open properties, andOpenFrame(*frame)namedtuple unpacking). On a short list this raisedIndexErrororTypeError. This is the receiver side of Azure/amqpnetlite#645: a spec-compliant sender that omits trailing nulls triggers the crash, while brokers and other AMQP stacks that bound decoding by the wire count are unaffected.Changes
decode_frameup to the performative's full field count, derived from each performative's_definitionand excluding the transfer payload slot, before any positional access or namedtuple unpacking, so omitted trailing fields read back asNone.list0(0x45) body encoding indecode_frame. A performative whose fields are all omitted may arrive aslist0, which carries no count byte; the previous code readdata[5]out of range and raisedIndexErrorbefore padding. It is now decoded as zero fields and padded to the full field count, closing the fully-omitted case for End and Close.protected-accesspylint warning and the mypyattr-definederror on the_PERFORMATIVE_FIELD_COUNTcomprehension's access to_code/_definition, matching the existing suppressions on every such access in_encode.py._pyamqpcopies, which remain byte-identical.list0End and Close encodings, and the per-performative field counts.Test plan
pytest tests/pyamqp_tests/unittest/test_decode.py, 17 passed.pytest tests/unittests/test_pyamqp_decode.py, 13 passed.pylint --enable=protected-accesson_decode.pyrates 10.00/10 with nouseless-suppressionwarning.