Skip to content

avoid integer overflow in container size precheck#3590

Open
dxbjavid wants to merge 1 commit into
apache:masterfrom
dxbjavid:container-size-overflow
Open

avoid integer overflow in container size precheck#3590
dxbjavid wants to merge 1 commit into
apache:masterfrom
dxbjavid:container-size-overflow

Conversation

@dxbjavid

@dxbjavid dxbjavid commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

the container size precheck multiplies a wire-controlled element count by the per-element minimum size in 32-bit int, so a crafted list, set or map header can wrap the product down to a small or zero value and slip past the maxMessageSize guard, letting an oversized container through before any allocation is bounded. the same shape sits in the binary, compact and json protocols and the protocol base, plus the c_glib binary and compact readers. widen the multiplication to the long type checkReadBytesAvailable already takes so the count can no longer overflow, and add a regression test for the three c++ protocols.

@mergeable mergeable Bot added c_glib c++ Pull requests that update C++ code labels Jun 9, 2026
@Jens-G

Jens-G commented Jun 9, 2026

Copy link
Copy Markdown
Member

Code review

Found 2 issues:

  1. static_cast<long> and (glong) are 32-bit on Windows/MSVC (LLP64 data model), the same width as int, so the multiplication still overflows there. The fix works on POSIX (LP64) where long is 64-bit, but not on Windows. Use int64_t/gint64 instead (and update TTransport::checkReadBytesAvailable's signature accordingly).

void checkReadBytesAvailable(TSet& set) override
{
trans_->checkReadBytesAvailable(static_cast<long>(set.size_) * getMinSerializedSize(set.elemType_));
}
void checkReadBytesAvailable(TList& list) override
{
trans_->checkReadBytesAvailable(static_cast<long>(list.size_) * getMinSerializedSize(list.elemType_));
}
void checkReadBytesAvailable(TMap& map) override
{
int elmSize = getMinSerializedSize(map.keyType_) + getMinSerializedSize(map.valueType_);
trans_->checkReadBytesAvailable(static_cast<long>(map.size_) * elmSize);
}
protected:
template <typename StrType>

The same pattern appears in TCompactProtocol.h, TJSONProtocol.h, TProtocol.h, thrift_binary_protocol.c, and thrift_compact_protocol.c.

  1. PR title and commit message are missing the required THRIFT-NNNN: prefix and Client: cpp,c_glib trailer required by AGENTS.md §2 for non-trivial changes. Please file a JIRA ticket at https://issues.apache.org/jira/browse/THRIFT and update the PR title and commit message.

🤖 Generated with Claude Code

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

the container size precheck multiplied a wire-controlled element count by
the per-element minimum size in 32-bit arithmetic, so a crafted list, set
or map header could wrap the product down to a small or zero value and slip
past the maxMessageSize guard before any allocation was bounded.

widen the multiplication to a fixed-width 64-bit type and widen the
checkReadBytesAvailable parameter to match, in both the c++ protocols and
the c_glib binary and compact readers, so the product can no longer overflow
on LLP64 platforms such as Windows where long is only 32-bit. regression
tests cover the three c++ protocols.

Client: cpp,c_glib
@dxbjavid dxbjavid force-pushed the container-size-overflow branch from 068af36 to 41cb98a Compare June 11, 2026 04:45
@dxbjavid

Copy link
Copy Markdown
Contributor Author

good catch on the width, you're right that long and glong stay 32-bit under LLP64 so the product would still wrap on MSVC. i've switched the casts to int64_t in the c++ protocols and gint64 in the c_glib binary and compact readers, and widened the checkReadBytesAvailable parameter to match in both TTransport and the c_glib transport vtable, since otherwise the wider cast just gets truncated back through the glong parameter when the value is passed in. rechecked the binary list case and an oversized header is still rejected against a 1 KiB maxMessageSize after the change.

on the second point, i've added the Client: cpp,c_glib trailer to the commit. i'll file a JIRA ticket for this and update the PR title and commit with the THRIFT- prefix once i have the number.

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

Labels

c_glib c++ Pull requests that update C++ code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants