Skip to content

THRIFT-2462: Add netstd recursion-depth round-trip regression test#3563

Open
Jens-G wants to merge 1 commit into
apache:masterfrom
Jens-G:netstd-recursion-depth
Open

THRIFT-2462: Add netstd recursion-depth round-trip regression test#3563
Jens-G wants to merge 1 commit into
apache:masterfrom
Jens-G:netstd-recursion-depth

Conversation

@Jens-G
Copy link
Copy Markdown
Member

@Jens-G Jens-G commented May 28, 2026

Summary

Validation of the recursion-depth handling for netstd showed that the limit is already enforced on the real (generated-code) read/write path: the netstd code generator emits IncrementRecursionDepth() / DecrementRecursionDepth() around the body of every generated WriteAsync / ReadAsync (see compiler/cpp/src/thrift/generate/t_netstd_generator.cc). Nested structs — both known fields (generated code) and unknown fields (TProtocolUtil.SkipAsync) — are therefore already bounded by TConfiguration.RecursionLimit (default 64).

An earlier revision of this PR additionally called IncrementRecursionDepth / DecrementRecursionDepth inside the protocols' WriteStructBegin/End and ReadStructBegin/End. Because the generated code already does this, that counted every struct twice, halving the effective limit and rejecting valid payloads at roughly limit/2. That change has been dropped — the three protocol files are now unchanged from master.

What remains is a round-trip regression test that exercises the limit through the generated WriteAsync / ReadAsync path (not the protocol *StructBegin/End methods in isolation), modeled on the Delphi test added in 1ffdcf2.

Changes

  • lib/netstd/Tests/Thrift.Tests/Protocols/TProtocolRecursionDepthTests.cs — new round-trip test over the mutually recursive CoRec / CoRec2 chain and the wide RecTree from test/Recursive.thrift, for Binary / Compact / JSON:
    • below-limit and at-limit chains round-trip successfully (off-by-one guard),
    • over-limit chains are rejected on both write and read with TProtocolException(DEPTH_LIMIT),
    • a wide tree of siblings round-trips — guards correct DecrementRecursionDepth,
    • a cyclic object graph is rejected instead of recursing forever.
  • Thrift.Compile.net10.csproj — generate test/Recursive.thrift so the recursive types are available to the test assembly.

No production code changes.

Test plan

  • dotnet test (Thrift.Tests): 18 / 18 pass.
  • Confirmed the test guards the real behavior: re-applying the earlier double-counting protocol change makes 9 / 18 fail (the below-/at-limit round-trips); with master's protocol code all 18 pass.

Co-authored with Claude Opus 4.8.

@mergeable mergeable Bot added the c# Pull requests that update C# code label May 28, 2026
@Jens-G Jens-G force-pushed the netstd-recursion-depth branch from a01f3a4 to be60e46 Compare May 28, 2026 14:10
@Jens-G Jens-G marked this pull request as draft May 28, 2026 22:44
Client: netstd

Recursion depth is already enforced on the generated read/write path: the
netstd code generator emits IncrementRecursionDepth/DecrementRecursionDepth
around the body of every generated WriteAsync/ReadAsync. This adds a round-trip
regression test (Binary/Compact/JSON) over nested structs from Recursive.thrift
to confirm the limit keeps being enforced, and wires Recursive.thrift into the
net10 compile-test project so the generated types are available.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Jens-G Jens-G force-pushed the netstd-recursion-depth branch from be60e46 to cf07bab Compare May 29, 2026 10:17
@Jens-G Jens-G changed the title THRIFT-2462: Harden netstd protocol recursion depth for struct read/write THRIFT-2462: Add netstd recursion-depth round-trip regression test May 29, 2026
@Jens-G Jens-G marked this pull request as ready for review May 29, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c# Pull requests that update C# code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant