Skip to content

[py] reject negative enum and union index in binary decoder#3876

Closed
arib06 wants to merge 1 commit into
apache:mainfrom
arib06:py-decoder-negative-index
Closed

[py] reject negative enum and union index in binary decoder#3876
arib06 wants to merge 1 commit into
apache:mainfrom
arib06:py-decoder-negative-index

Conversation

@arib06

@arib06 arib06 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What is the purpose of the change

read_enum, read_union and skip_union in the Python binary decoder read the enum symbol index / union branch index from the wire and validate it with an upper-bound-only check (index >= len(...)). A negative index is not caught. Because the index is then used directly to subscript writers_schema.symbols / writers_schema.schemas, and Python lists accept negative subscripts, a crafted negative zigzag index passes validation and wraps to an element counted from the end of the list. For an enum this silently returns the wrong symbol; for a union it selects a branch that the writer never declared and decodes the following bytes with that wrong branch schema, which is a decode-time type confusion and desyncs the stream for skip_union. Positive out-of-range indices are already rejected, so this only closes the negative side. The check becomes 0 <= index < len(...) at all three sites.

Verifying this change

This change added tests and can be verified as follows:

  • Added test_negative_enum_index and test_negative_union_index in TestMisc, feeding a single 0x01 byte (zigzag -1) as the index. Both assert SchemaResolutionException. Against the unpatched decoder the enum read returns symbols[-1] and the union read decodes via schemas[-1], so both tests fail before the fix.
  • Full test_io suite green (152 tests).

Documentation

  • Does this pull request introduce a new feature? no

@RyanSkraba

Copy link
Copy Markdown
Contributor

Ooof, my apologies: it looks like these changes are all contained in #3861 submitted last week. It adds a lot of additional detailed validation around potentially invalid data, and it seems like the right PR to merge.

Please don't be discouraged, there's plenty of bugs remaining to fix 😄 For what it works, this one LGTM.

@RyanSkraba RyanSkraba closed this Jul 19, 2026
@arib06

arib06 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@RyanSkraba understood, and thankyou for informing. i will be looking forward to keep on contributing .

@RyanSkraba

Copy link
Copy Markdown
Contributor

If you're a strong python developer, we'd love some help on PR reviews (including #3861)

@arib06

arib06 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Happy to help on reviews. Left comments on #3861 — the index checks there cover the same three sites this PR did, plus a lot more, so no complaint about closing this one. Flagged one perf regression in the collection path with numbers and a suggested fix, otherwise it looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants