[
https://issues.apache.org/jira/browse/THRIFT-6291?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-6291:
-------------------------------
Description:
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._
was:
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._
> 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
> Priority: Minor
> Time Spent: 10m
> 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)
