[
https://issues.apache.org/jira/browse/THRIFT-6291?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer resolved THRIFT-6291.
--------------------------------
Fix Version/s: 0.26.0
Assignee: Jens Geyer
Resolution: Fixed
> Align the container-size limit across the TConfiguration bindings
> -----------------------------------------------------------------
>
> Key: THRIFT-6291
> URL: https://issues.apache.org/jira/browse/THRIFT-6291
> Project: Thrift
> Issue Type: Improvement
> Components: Documentation
> Reporter: Jens Geyer
> Assignee: Jens Geyer
> Priority: Minor
> Fix For: 0.26.0
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> Eight bindings let a caller bound the declared element count of a list, set
> or map before the decoder sizes the collection. Three do not, and where the
> knob exists it is spelled five different ways. The deliverable of this issue
> is the specification update below, which records the shape the project
> follows and settles the conventions. Closing the gap in the three bindings
> that lack the setting is done in its sub-tasks, one pull request per language.
> h2. Current state (read-verified on master)
> || Binding || Container-count limit || Where || Default ||
> | c_glib | yes | {{container_limit}} property, binary + compact protocol and
> their factories | {{0}} = off |
> | C++ | yes | {{TBinaryProtocolT::setContainerSizeLimit()}}, likewise compact
> | {{0}} = off |
> | Java | yes | {{containerLengthLimit_}} ctor argument | {{-1}}
> ({{NO_LENGTH_LIMIT}}) = off |
> | Python | yes | {{container_length_limit}} on binary, compact and JSON, and
> their factories | {{None}} = off |
> | D | yes | {{containerSizeLimit}} ctor argument | {{16384000}} |
> | Node.js | yes (compact) | {{container_limit_}} | {{0}} = off |
> | Rust | yes | {{TConfiguration::max_container_size}} | {{None}} = off |
> | Go | yes | {{TConfiguration.MaxContainerSize}} (THRIFT-6286) | {{<= 0}} =
> off |
> | netstd | *no* | (none) | (none) |
> | Haxe | *no* | (none) | (none) |
> | Delphi | *no* | (none) | (none) |
> h2. Why these three
> netstd, Haxe and Delphi are not unbounded. All three call
> {{CheckReadBytesAvailable(count * GetMinSerializedSize(elementType))}} in
> every {{ReadListBegin}} / {{ReadSetBegin}} / {{ReadMapBegin}} of the binary,
> compact and JSON protocols, and that call is charged against the remaining
> message budget, which is seeded from {{MaxMessageSize}}.
> That check bounds the container in *wire bytes*. It cannot bound it in
> *per-element in-memory footprint*, because the minimum serialized size of a
> struct element is 1 byte (the {{T_STOP}}). At the default {{MaxMessageSize}}
> of 100 MB a list of structs therefore passes the check at up to ~100 million
> elements, each of which becomes a full object on the heap.
> {{doc/thrift-threat-model.md}} makes the same point in its appendix: the
> {{checkReadBytesAvailable}} path and the container-limit path are independent
> guards, and the message-size cap alone does not bound memory when the element
> type has a large in-memory footprint.
> The other eight bindings give the operator a second lever for exactly that
> case; these three do not. That is the parity gap, and it is the only one —
> the bindings in the first group need no change.
> h2. Shape to follow
> * The three bindings already own a {{TConfiguration}}, and their protocols
> already take every other limit from it, so the new setting belongs there
> rather than on new protocol constructors or setters. This follows Rust and Go
> (THRIFT-6286).
> * Bindings whose knob is already on the protocol (c_glib, C++, Java, Python,
> D, Node.js) keep it. Mirroring it into {{TConfiguration}} would leave two
> ways to set one limit and buys no coverage. Whether each of those knobs is
> actually *reachable* on every path that sizes a collection is a separate
> question, tracked outside this issue.
> * *Off by default*, so no message that is accepted today is rejected
> afterwards.
> * One insertion point per binding: the three shared
> {{CheckReadBytesAvailable}} helpers on the protocol base class already carry
> the element count for all three protocols.
> h2. Scope: container count only, not string length
> {{MaxMessageSize}} already bounds a single string, because a string's
> declared length is its byte length and the remaining-bytes check sees it
> directly. The container count is the case the byte budget cannot see, since
> the multiplier is per-element in-memory footprint. A string-length setting
> would therefore be a second spelling of a limit that is already effective,
> and is deliberately out of scope here. Bindings that do expose a string limit
> as well (C++, Java, Python, D, c_glib, Rust) did so for reasons outside this
> issue; nothing here proposes removing those.
> h2. Spec
> {{doc/specs/thrift-tconfiguration.md}} still reads "currently holds three
> settings" and was last revised in 2019. THRIFT-5021 was filed because limits
> were, in that document's own words, "not consistently managed and just
> randomly distributed across the code base", so adding a fourth setting to
> several bindings without writing it down would reproduce the condition the
> spec exists to prevent. This issue carries that update, and it settles two
> things the tree is currently inconsistent about:
> * *The off sentinel.* Four spellings are in use: {{0}} (C++, c_glib,
> Node.js), {{-1}} (Java), {{None}} (Rust), {{<= 0}} (Go). The spec should name
> one for the new setting.
> * *The default.* It is not uniformly off today — D ships a finite container
> limit of {{16384000}}, Python a finite {{string_length_limit}}, and Rust a
> finite {{max_string_size}} of 100 MB. The spec should state the rule for the
> new setting and note D as the pre-existing exception rather than imply a
> uniformity that does not exist.
> h2. Sub-tasks
> One per binding, each its own pull request: netstd, Haxe, Delphi. They depend
> on this issue only for the two conventions above; the code change itself is
> independent in each.
> _Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens
> Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
