Skip to content

Reject truncated Thrift containers before allocation - #18631

Open
HTHou wants to merge 3 commits into
apache:masterfrom
HTHou:codex/fix-thrift-container-allocation
Open

Reject truncated Thrift containers before allocation#18631
HTHou wants to merge 3 commits into
apache:masterfrom
HTHou:codex/fix-thrift-container-allocation

Conversation

@HTHou

@HTHou HTHou commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Purpose

Reject malformed Thrift RPC frames that declare a container requiring more bytes than remain in the current frame, before generated code allocates the container.

Changes

  • Compare Thrift's minimum encoded size estimate with the bytes remaining in the current TElasticFramedTransport frame.
  • Reject truncated list, set, map, string, and binary payloads with a corrupted-data transport error.
  • Remove the fixed thrift_container_length_limit configuration and the broad protocol-level container limit.
  • Keep complete large requests valid; the regression test decodes a request containing 200001 actual elements across successive frames.

Compatibility

This does not impose a fixed element-count limit. Valid containers remain subject to the existing maximum frame size, while malformed/truncated frames are rejected before their declared container size can trigger an oversized allocation. A malformed frame causes the transport connection to close, consistent with existing frame-protection errors.

Tests

  • mvn -o test -pl iotdb-client/service-rpc -am -Dtest=TElasticFramedTransportReadTest,TElasticFramedTransportTest -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false
  • mvn -o test-compile -DskipTests
  • mvn -o test-compile -P with-zh-locale -DskipTests

@HTHou
HTHou marked this pull request as ready for review September 14, 2026 03:11
@HTHou
HTHou requested a balanced review from Copilot September 14, 2026 03:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The implementation consistently applies the validated limit and includes focused regression coverage.

Pull request overview

Adds configurable server-side Thrift container decoding limits to reduce resource-exhaustion risk.

Changes:

  • Adds a validated 100,000-element default limit.
  • Applies the limit to binary and compact Thrift protocols.
  • Adds configuration entries and regression coverage.
File summaries
File Description
RpcUtils.java Defines the default limit.
CommonConfig.java Stores and normalizes the setting.
CommonDescriptor.java Loads the configuration property.
AbstractThriftServiceThread.java Applies the limit to protocol factories.
AbstractThriftServiceThreadTest.java Tests both protocol encodings.
iotdb-system.properties.template Documents the setting.
iotdb-system.properties Adds the standard default.
edge/iotdb-system.properties Adds the Edge default.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@HTHou
HTHou marked this pull request as draft September 14, 2026 03:26
@HTHou
HTHou marked this pull request as ready for review September 14, 2026 03:28
@HTHou HTHou changed the title Bound Thrift container sizes during RPC decoding Reject truncated Thrift containers before allocation Sep 14, 2026
@HTHou
HTHou marked this pull request as draft September 14, 2026 04:41
@HTHou
HTHou requested a balanced review from Copilot September 14, 2026 04:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The validation is narrowly scoped and comprehensively tested across supported protocol and compression combinations.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@HTHou
HTHou marked this pull request as ready for review September 14, 2026 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants