Jens Geyer created THRIFT-6291:
----------------------------------
Summary: 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
Seven 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 |
| 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 seven 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, D,
Node.js) keep it. Mirroring it into {{TConfiguration}} would leave two ways to
set one limit and buys no coverage.
* *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, 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}} and Rust ships 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)
