[EventHub][ServiceBus] Fix pyAMQP decode of performatives with omitted trailing null fields - #47660
Conversation
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. |
6fecb8a to
f89b410
Compare
fbd200d to
50c8857
Compare
Eldert Grootenboer (EldertGrootenboer)
left a comment
Eldert Grootenboer (EldertGrootenboer)
left a comment
There was a problem hiding this comment.
lgtm
61bf6dc to
a3e67e3
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.
…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.
a3e67e3 to
3accb70
Compare
Josue Gomez (SwayGom)
left a comment
Josue Gomez (SwayGom)
left a comment
There was a problem hiding this comment.
Approving. I reviewed this as untrusted-network-input parsing rather than as a normal bug fix, and it holds up on every axis I care about there.
Defaults table verified entry-by-entry against the OASIS spec. I imported the patched module, dumped _PERFORMATIVE_FIELD_DEFAULTS, and checked each against the AMQP 1.0 transport definitions: Open (max_frame_size=4294967295, channel_max=65535), Begin (handle_max=4294967295), Attach (snd_settle_mode=2, rcv_settle_mode=0, incomplete_unsettled=False), Flow (drain/echo=False), Transfer (more/resume/aborted/batchable=False), Disposition, Detach, End/Close, and the SASL frames. All correct. The if field is not None filter correctly excludes only the Transfer payload sentinel.
The two properties that make the explicit-null substitution safe both hold. The guard at _decode.py:509 is if default is not None and fields[index] is None:
- Fields with a null default are never touched.
Attach.initial_delivery_count(frame[9]) reads backNonefor both the omitted and explicit-null cases — which is load-bearing, sincereceiver.py:49andaio/_receiver_async.py:52doif frame[9] is None: … Detaching link. That detection is preserved. - Only
Noneis replaced, never falsy values. An explicitly encodedmax_frame_size=0stays0, so_connection.py:440'sframe[2] < 512still rejects it. A naiveif not fields[index]would have broken this; the code does the right thing.
On the spec question, §1.4 says "When the trailing elements of the list representation are null, they MAY be omitted", and the worked example states a trailing explicit null "can optionally be omitted according to the encoding rules" — so explicit-null and omitted denote the same value, and §1.3.4 defines default as the value "if no value is encoded." Collapsing both to the default is the interoperable reading. I walked every positional consumer in both the sync and async engines (_connection.py, session.py, link.py, sender.py, receiver.py, sasl.py and the aio/ mirrors) and found no caller that distinguishes explicitly-null from absent for any field with a non-null default.
Hostile-input matrix run against the patched decoder. Padding is bounded by len(field_defaults) ≤ 14, so no unbounded allocation; the pre-existing _MAX_COMPOUND_COUNT cap still fires (count=70000 → ValueError); the list0 path is safe (data[4:] on a 4-byte frame yields an empty memoryview, count=0 skips the loop) and I confirmed it for End/Close/Transfer plus an unknown descriptor (0x99 → (153, []), no crash).
Vendored copies are genuinely identical. The diff itself proves it for _decode.py — both files show index 32ef0ddd9c12..68e571a45c56, i.e. identical pre- and post-image blob hashes. I also enumerated all 39 files in both _pyamqp trees at head SHA and compared blob SHAs: zero differing files. The guard test compares content, not existence.
Every new test corresponds to a real pre-fix failure. I reverted _decode.py to the base blob and re-ran: short Open unpack → TypeError: missing 9 required positional arguments; short Open frame[2] < 512 → IndexError; explicit-null max_frame_size → TypeError: '<' not supported between NoneType and int; short Transfer frame[11] → IndexError; list0 End/Close → IndexError: index out of bounds on dimension 1; short SASLOutcome → TypeError. All pass post-fix. Existing test_decode_bounds.py still passes (35 tests).
Nice incidental catch: sasl.py:89 / aio/_sasl_async.py:93 do "…{}…{}".format(*fields) on a SASLOutcome, which used to raise IndexError when a broker omitted additional_data. That's fixed too.
Both suppressions are necessary and correctly scoped. Removing the pylint: disable=protected-access,no-member produces W0212 plus 14× E1101 (the attributes are attached dynamically in performatives.py). I also checked whether a disable inside a dict-comprehension leaks module-wide — it does not; a later OpenFrame._code access in the same file is still flagged. Removing the type: ignore produces two union-attr errors, and --warn-unused-ignores doesn't flag it as redundant. pylint rates the patched file 10.00/10.
Version and changelog are consistent — eventhub is already at 5.15.2 (Unreleased) so the entry correctly appends rather than re-bumping; servicebus lands in 7.15.0 (Unreleased). Neither entry overclaims.
Three minor items, none blocking.
1. Over-long wire count is neither rejected nor truncated — _decode.py:497-499. fields.extend(field_defaults[count:]) is a no-op when count > len(field_defaults), so a peer can send a Close with count=3: decode_frame returns (24, [None, None, None]) and CloseFrame(*fields) raises TypeError: takes 2 positional arguments but 4 were given. For Transfer with count=12 the payload lands at index 12 while receiver.py:70/77 hardcode frame[11], so decode_payload gets a decoded field instead of the payload.
Behaviour is identical pre- and post-fix, so this isn't introduced here. But since the PR is explicitly about making this function total over adversarial framings, a symmetric if count > len(field_defaults): raise ValueError(...) would close the other half and turn a TypeError deep in a namedtuple constructor into a clean protocol error. Same reasoning applies to the else branch at :483, which treats any non-0xd0/0x45 constructor as list8 — a 0x40 or 0xa1 body reads data[5] as a count.
2. The drift guard hard-fails instead of skipping outside a repo layout — test_pyamqp_decode.py:209 and its eventhub twin. The stated intent is to skip when the sibling source isn't present, but the assert root is not None above the pytest.skip defeats it: with no ancestor directory named sdk, the test fails. I reproduced this outside the repo tree — 3 failures, other 22 pass. In-repo CI resolves the root fine, so it's latent. pytest.skip(...) when root is None would match the documented intent.
3. The drift guard covers 3 of 39 vendored files — the parametrize list is ["_decode.py", "_encode.py", "performatives.py"]. All 39 are currently identical, so the guard could walk both trees; as written, a divergence in session.py, link.py, or anything under aio/ passes silently while looking like drift protection.
One informational note, not a defect here: TransferFrame._definition declares message_format default 0, but the spec's transfer definition has no default attribute on message-format. That's a pre-existing pyAMQP-ism the new table simply inherits, and 0 is the only defined format in practice.
Not verified: I ran only the pyAMQP unit tests against a standalone copy at head SHA, not the live/recorded integration suites. I also didn't identify which broker in the wild emits the short performatives that motivated this — though the change is correct on spec grounds regardless.
Summary
The pyAMQP transport crashes when it decodes an AMQP performative from a sender that omitted trailing fields or encoded a field as an explicit null. The decoder now pads the decoded field list to the full field count and applies each field's AMQP default. Positional field access and namedtuple unpacking are safe, and fields read back as their declared defaults.
Motivation
AMQP 1.0 section 1.4 permits a sender to omit the trailing fields of a performative when their values equal the defaults. An incoming performative can thus be shorter than its full field count. The decoder in
_decode.pybuilt the field list from the element count on the wire. Downstream code then reads fixed positions, for exampleframe[9]for Attachinitial_delivery_countandOpenFrame(*frame)namedtuple unpacking. On a short list this raisedIndexErrororTypeError.Some fields have a non-null default.
_incoming_opencomputesframe[2] < 512onmax_frame_size, whose default is 4294967295. If the decoder pads that field with null, the comparison raisesTypeError. The same failure occurs with an explicit null on the wire. Only trailing fields can be omitted, so a sender that sets a later field and keeps the default of an earlier field must encode the earlier field as an explicit null. The decoder must map that null back to the default.This is the receiver side of Azure/amqpnetlite#645. A spec-compliant sender that omits trailing fields makes the receiver crash. Brokers and AMQP stacks that bound decoding by the wire count are not affected.
Changes
decode_framepads the decoded performative field list to the full field count of the performative. The pad values come from each field's declared default in the performative's_definition. A field with a non-null default (for examplemax_frame_size,channel_max, or the Transfer boolean flags) reads back as that default. A field with a null default reads back asNone.decode_framereplaces a decoded explicit null with the field's declared default when that default is non-null. An explicit null now reads back the same as an omitted field. Fields with a null default (for examplehostnameandinitial_delivery_count) stayNone.decode_framehandles the AMQPlist0(0x45) body encoding. A performative with all fields omitted can arrive aslist0, which carries no count byte. The previous code readdata[5]out of range and raisedIndexError. The decoder now treatslist0as zero fields and pads to the full field count._PERFORMATIVE_FIELD_DEFAULTScomprehension carriespylint: disable=protected-access,no-memberand a mypytype: ignorefor its access to_codeand_definition. These attributes are set on the performative classes at import time, so static analysis cannot see them. This matches the existing suppressions in_encode.py._pyamqpcopies. The two copies stay byte-identical, and a test guards against drift.max_frame_sizewith a later field set, a short Transfer with a trailing payload, the performatives without namedtuple defaults, thelist32body encoding, short SASL frames, thelist0End and Close encodings, and the per-performative field counts.api.mdandapi.metadata.ymlfor azure-eventhub are added, generated withazpysdk apistub(parser 0.3.28), to satisfy the API consistency check.Test plan
pytest tests/pyamqp_tests/unittest/test_decode.py, 32 passed.pytest tests/unittests/test_pyamqp_decode.py, 25 passed.pylinton_decode.pyreports zero errors with the CI plugin versions (pylint 4.0.4, azure-pylint-guidelines-checker 0.5.7).