Skip to content

THRIFT-6051: Limit struct read/write recursion depth in OCaml library#3556

Merged
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:ocaml-recursion-depth
Jun 3, 2026
Merged

THRIFT-6051: Limit struct read/write recursion depth in OCaml library#3556
Jens-G merged 1 commit into
apache:masterfrom
Jens-G:ocaml-recursion-depth

Conversation

@Jens-G
Copy link
Copy Markdown
Member

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

Summary

Adds a struct read/write recursion-depth limit (default 64) to the OCaml library, matching the other languages in this series.

  • lib/ocaml/src/Thrift.ml: add recursion_depth_ plus increment_recursion_depth / decrement_recursion_depth to Protocol.t; the increment raises Protocol.E(DEPTH_LIMIT, …) once the depth exceeds 64.
  • Generator (t_ocaml_generator.cc): wrap every generated struct reader and writer with increment_recursion_depth; Fun.protect ~finally:decrement_recursion_depth (fun () -> …), so the counter is always restored, even when an exception unwinds. The OCaml generator snapshot test (compiler/cpp/tests/ocaml/snapshot_service_handle_ex.hpp, run by ThriftCompilerTests) is updated to match the guarded output.
  • Also fixes a pre-existing forward-reference bug that left the library uncompilable on modern OCaml: Protocol.E (Protocol.INVALID_DATA, …) (introduced in dbc1f8def) referenced the Protocol module from inside its own definition. exn_type / exception E are moved ahead of class virtual t and the reference is unqualified.

Union & exception coverage

In OCaml there is no separate union/exception codegen: the base generator routes unions through generate_struct and exceptions through generate_xception, both reaching the single generate_ocaml_struct_definition reader/writer. The guard therefore applies uniformly to all three — verified in generated output (read_recTree, read_coUnion, read_coError and every write carry the identical wrapper).

Test

lib/ocaml/test/test_recursion_depth.ml now performs full read/write round-trips (not isolated counter calls) for a recursive struct, union, and exception, against an in-memory protocol:

  • a 64-deep chain round-trips and preserves its depth;
  • writing a 65-deep chain raises DEPTH_LIMIT;
  • reading a crafted 65-deep payload raises DEPTH_LIMIT.

Build & run (there is no dune in the repo; the OCaml lib pre-dates it):

make -C lib/ocaml/test

Result: 10 passed, 0 failed (validated against OCaml 4.13.1).

Confirmed the problem exists on master: an unguarded (old-generator-style) reader accepts a 100-deep payload with no limit, whereas the guarded reader raises DEPTH_LIMIT at depth 65.

Why inline types instead of test/Recursive.thrift

The shared recursive types cannot be exercised through generated OCaml here, for two pre-existing, out-of-scope reasons (neither related to THRIFT-6051):

  1. The generator emits mutually-recursive classes (CoRec/CoRec2, CoUnion/CoUnion2, CoError/CoError2) as separate class declarations instead of class … and …, so the generated module fails to compile (Unbound type constructor coRec2).
  2. TBinaryProtocol.ml does not compile on modern OCaml (uses removed mutable-string APIs; the compiler is -force-safe-string).

The test's hand-written types mirror the generator's exact (verified) output, and the in-memory protocol stands in for TBinaryProtocol. There is currently no OCaml CI, so this test is run manually.

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

@Jens-G Jens-G requested review from fishy and mhlakhani as code owners May 28, 2026 11:46
@Jens-G Jens-G marked this pull request as draft May 28, 2026 22:45
@Jens-G Jens-G force-pushed the ocaml-recursion-depth branch from efefe5d to 82b706c Compare June 1, 2026 08:23
Client: ocaml

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Jens-G Jens-G force-pushed the ocaml-recursion-depth branch from 82b706c to 9626050 Compare June 1, 2026 08:41
@Jens-G Jens-G marked this pull request as ready for review June 1, 2026 09:11
@Jens-G Jens-G merged commit eaeb208 into apache:master Jun 3, 2026
88 of 89 checks passed
@Jens-G Jens-G deleted the ocaml-recursion-depth branch June 3, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant