Skip to content

enforce max_string_size on non-strict binary message name - #3609

Merged
Jens-G merged 1 commit into
apache:masterfrom
dxbjavid:rs-binary-message-name-limit
Jul 1, 2026
Merged

enforce max_string_size on non-strict binary message name#3609
Jens-G merged 1 commit into
apache:masterfrom
dxbjavid:rs-binary-message-name-limit

Conversation

@dxbjavid

@dxbjavid dxbjavid commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

the rust binary protocol reader routes every length-prefixed value through the configured max_string_size, except one. in the non-strict read_message_begin path (the legacy mode kept for senders that don't write the protocol-version header) the leading four bytes are taken as the message-name length and the name buffer is allocated straight from that wire value, without the check that read_bytes and read_string both apply. this change runs the name length through the same max_string_size check before allocating and returns a SizeLimit error when it is exceeded. the compact reader is unaffected because its read_message_begin goes through read_string, which is already bounded.

kept the change inside read_message_begin so the rest of the path is unchanged. tests: a non-strict name over the limit is now rejected with SizeLimit, and a name of exactly the configured limit is still accepted so the boundary is > and not >=.

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@mergeable mergeable Bot added the rust Pull requests that update Rust code label Jun 29, 2026
@Jens-G

Jens-G commented Jun 30, 2026

Copy link
Copy Markdown
Member

Code review

Three process issues (AGENTS.md compliance); no logic defects verified.

  1. PR description language — the PR body describes the class of issue being addressed using framing that should not appear in public artifacts (AGENTS.md §6: "Never describe the change as a security fix in public-facing text — commit messages, PR titles, PR descriptions, or inline comments. Use neutral functional language."). A functional description such as "enforce the string size limit on the non-strict `read_message_begin` path" would be appropriate.

  2. Threat-model cross-check (AGENTS.md §6) — the change adds enforcement to a serialization-bounds path. AGENTS.md §6 requires that changes touching serialization bounds be cross-checked against `doc/thrift-threat-model.md` before merging.


Coverage note (UNVERIFIABLE): `must_enforce_string_size_limit_on_non_strict_message_name` tests that a 2000-byte name is rejected at a limit of 1000. There is no test for the passing side: a name of exactly 1000 bytes should be accepted (`>` not `>=`). The implementation is correct at this boundary — a two-state proof cannot be constructed (a correct-boundary test would pass on both PR and base code) — so this is recorded as a coverage suggestion only, not a verified defect.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Jens-G
Jens-G self-requested a review June 30, 2026 16:57

@Jens-G Jens-G left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See "coverage note" above. Pls comment and/or consider improving the tests.

@dxbjavid
dxbjavid force-pushed the rs-binary-message-name-limit branch from b554870 to 314d331 Compare July 1, 2026 16:13
@dxbjavid

dxbjavid commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

good points, all three addressed.

reworded the PR description to stick to neutral functional language, so it now says what the code does rather than framing the class of the issue.

on the threat-model cross-check: i went through doc/thrift-threat-model.md. this doesn't move any trust boundary or change a default. the client peer is already untrusted by the model, and §9 D3 records that the default container/string limit is INT32_MAX and that operators are expected to set finite limits before exposing a server. all this does is make the non-strict message name honour the max_string_size the operator has already configured, the same as read_bytes and read_string do, so the non-strict path stops being a gap in that operator-facing control.

on the coverage note: added must_allow_non_strict_message_name_at_limit, which feeds a non-strict name of exactly the configured limit and asserts it is accepted (name, type and seq all read back). so the boundary is pinned on both sides now, > and not >=. full lib suite passes, fmt clean.

@Jens-G
Jens-G self-requested a review July 1, 2026 20:51
@Jens-G
Jens-G merged commit 0ab16e3 into apache:master Jul 1, 2026
87 of 88 checks passed
@dxbjavid

dxbjavid commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @Jens-G for reviewing and merging the PR. Appreciated.

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

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants