GH-51757: [C++] Limit max JSON nesting depth in JSON parser by pitrou · Pull Request #51758 · apache/arrow

pitrou · 2026-10-05T13:25:26Z

Rationale for this change

Our JSON parser is inherently recursive, and a crafted JSON document with huge nesting can send it into a stack overflow.

Given that real-world JSON files will not have thousands levels of nesting, we limit the JSON nesting to an arbitrary value of 300.

Are these changes tested?

Yes, by new tests.

Are there any user-facing changes?

This PR contains a "Critical Fix". A crafted JSON document with a large nesting depth could trigger a denial of service through stack overflow.

Was AI used for this PR?

PR code and description written by:

Human
AI

Reviewed before submission by:

Human (self-reviewed)
AI
Not reviewed

GitHub Issue: [C++] Limit JSON max nesting depth #51757

pitrou · 2026-10-05T15:47:20Z

Hmm, the macOS Python builds don't like our new PyArrow tests. I think it's because the default stack size for new threads on macOS is quite smaller (512 kiB by default), debug builds might add some stack overhead, and some of our JSON reading tests are multi-threaded.

It's a bit unexpected that 512 kiB of stack isn't enough for 300 levels of nesting (the current limit). I'm now trying to make stack consumption a bit smaller still, by removing the functional overhead in RawBuilderSet::Finish.

pitrou · 2026-10-05T16:06:34Z

Hmm, the macOS Python builds don't like our new PyArrow tests. I think it's because the default stack size for new threads on macOS is quite smaller (512 kiB by default), debug builds might add some stack overhead, and some of our JSON reading tests are multi-threaded.

It's a bit unexpected that 512 kiB of stack isn't enough for 300 levels of nesting (the current limit). I'm now trying to make stack consumption a bit smaller still, by removing the functional overhead in RawBuilderSet::Finish.

Hmm, still getting crashes on macOS Python. Does anyone have a macOS machine so as to get a stack trace?

@taepper @Reranko05

pitrou · 2026-10-05T16:15:46Z

That said, it seems that stack consumption in debug mode on Linux can be huge as well (each nesting level in StructType::~StructType is about 1kB of stack space!). It's just that default stack size on Linux is much larger, 8 MB IIRC.

Perhaps we should just disable these PyArrow tests in debug mode on macOS.

pitrou · 2026-10-05T16:16:44Z

@github-actions crossbow submit wheelcp314*

pitrou · 2026-10-05T17:20:06Z

Ok, the PyArrow tests passed on the macOS wheel builds (in release mode), so regular users will get stack overflow protection.

pitrou · 2026-10-05T17:30:25Z

@taepper Do you want to review this?

Reranko05 · 2026-10-06T02:09:51Z

Should we also add an array variant in the C++ test, like the Python test has for both object and array? Right now the C++ test only covers nested objects.

pitrou · 2026-10-06T07:02:42Z

Should we also add an array variant in the C++ test, like the Python test has for both object and array? Right now the C++ test only covers nested objects.

Good point, I've added a C++ test for this.

Our JSON parser is inherently recursive, and a crafted JSON document with huge nesting can send it into a stack overflow. Given that real-world JSON files will not have thousands levels of nesting, we limit the JSON nesting to an arbitrary value of 1000.

pitrou · 2026-10-06T12:36:23Z

I've rebased after PR #51038 (migrating the parser to simdjson) was merged.

pitrou · 2026-10-06T12:39:05Z

@github-actions crossbow submit wheelcp314*

pitrou · 2026-10-06T12:39:17Z

@github-actions crossbow submit -g cpp

github-actions · 2026-10-06T12:41:46Z

Revision: 422930f

Submitted crossbow builds: ursacomputing/crossbow @ actions-af8e1837a0

Task	Status
wheel-macos-monterey-cp314-cp314-amd64
wheel-macos-monterey-cp314-cp314-arm64
wheel-macos-monterey-cp314-cp314t-amd64
wheel-macos-monterey-cp314-cp314t-arm64
wheel-manylinux-2-28-cp314-cp314-amd64
wheel-manylinux-2-28-cp314-cp314-arm64
wheel-manylinux-2-28-cp314-cp314t-amd64
wheel-manylinux-2-28-cp314-cp314t-arm64
wheel-musllinux-1-2-cp314-cp314-amd64
wheel-musllinux-1-2-cp314-cp314-arm64
wheel-musllinux-1-2-cp314-cp314t-amd64
wheel-musllinux-1-2-cp314-cp314t-arm64
wheel-windows-cp314-cp314-amd64
wheel-windows-cp314-cp314t-amd64

github-actions · 2026-10-06T12:42:13Z

Revision: 422930f

Submitted crossbow builds: ursacomputing/crossbow @ actions-c29d6a353c

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-debian-13-cpp-amd64
test-debian-13-cpp-i386
test-debian-experimental-cpp-gcc-15
test-fedora-42-cpp
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

taepper

Looks very good! I really like the refactoring of lambdas to explicitly destructing the child data at call sites.

One gap I found is the ConsumeJsonValue method, which has the same recursive behavior as the parser had.

taepper · 2026-10-06T13:04:06Z

In this case, we still call a recursive method without any depth limit. Ideally, we want to pass the current depth to ConsumeJsonValue.

This PR definitely already fixes many cases, so whether we want to change ConsumeJsonValue to accept a depth parameter in this same PR is a different question.

Alternatively, we can add a follow-up issue

Good point, I'll tackle this here.

We could solve this like in this case:
#51472

where we let simdjson handle the skip by the regular advancing of the iterator

But this would avoid the json validation (beyond structural errors regarding braces [,],{,} which will still be checked), which is probably required at this point

pitrou · 2026-10-06T14:09:48Z

+// An upper bound on the expected nesting depth of a geospatial JSON metadata object
+constexpr int kMaxJsonDepth = 20;


@paleolimbot Does this limit look ok?

Yes, I think that makes sense. Thank you!

taepper · 2026-10-06T14:18:16Z

+    // 100'000 would definitely trigger a stack overflow, validate that the error is
+    // detected before that would happen.


Note: in debug builds we probably did not run into stack overflows, but simdjson assertions.

simdjson::ondemand::parser also has a depth limit (default 1024), and when development checks are enabled, it will assert and thus possibly abort the program, if we consume objects deeper than that limit:

https://github.com/simdjson/simdjson/blob/7fa77b1e4ba2a21c97d97adc48e4cd8b9d1e7faa/doc/basics.md?plain=1#L3985-L4032

I did not quite understand the purpose of this limit (other than aborting debug code using the parser). I will open an issue in simdjson to find out a bit more.

Not any change request, but just for context, which we should maybe document somewhere?

pitrou · 2026-10-06T14:27:50Z

Ok, so musllinux crashes after ~152 levels of nesting. It seems the thread stack size is 128kiB there, and each level of nesting in ParseImpl::ParseValue takes 792 bytes of stack (!).

pitrou · 2026-10-06T14:48:03Z

@github-actions crossbow submit wheelcp314*

github-actions · 2026-10-06T14:50:44Z

Revision: e71ab61

Submitted crossbow builds: ursacomputing/crossbow @ actions-379dcb9fd6

Task	Status
wheel-macos-monterey-cp314-cp314-amd64
wheel-macos-monterey-cp314-cp314-arm64
wheel-macos-monterey-cp314-cp314t-amd64
wheel-macos-monterey-cp314-cp314t-arm64
wheel-manylinux-2-28-cp314-cp314-amd64
wheel-manylinux-2-28-cp314-cp314-arm64
wheel-manylinux-2-28-cp314-cp314t-amd64
wheel-manylinux-2-28-cp314-cp314t-arm64
wheel-musllinux-1-2-cp314-cp314-amd64
wheel-musllinux-1-2-cp314-cp314-arm64
wheel-musllinux-1-2-cp314-cp314t-amd64
wheel-musllinux-1-2-cp314-cp314t-arm64
wheel-windows-cp314-cp314-amd64
wheel-windows-cp314-cp314t-amd64

github-actions Bot added Component: C++ Component: Python awaiting review Awaiting review labels Oct 5, 2026

pitrou added Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. backport-candidate labels Oct 5, 2026

pitrou force-pushed the gh51757-json-max-nesting-depth branch from e3608fb to 3a59361 Compare October 5, 2026 14:18

This comment was marked as outdated.

Sign in to view

pitrou added the CI: Extra: C++ Run extra C++ CI label Oct 5, 2026

pitrou marked this pull request as ready for review October 5, 2026 17:20

pitrou requested review from AlenkaF, raulcd and rok as code owners October 5, 2026 17:20

Reranko05 reviewed Oct 6, 2026

View reviewed changes

Comment thread python/pyarrow/tests/test_json.py

Comment thread cpp/src/arrow/json/parser.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 6, 2026

pitrou force-pushed the gh51757-json-max-nesting-depth branch from 3e4a96c to 9ddab7c Compare October 6, 2026 07:02

pitrou force-pushed the gh51757-json-max-nesting-depth branch from 9ddab7c to 422930f Compare October 6, 2026 12:35

taepper reviewed Oct 6, 2026

View reviewed changes

Also guard ConsumeJsonValue against stack overflow

1d3da0d

pitrou requested review from HuaHuaY and wgtmac as code owners October 6, 2026 14:07

github-actions Bot added the Component: Parquet label Oct 6, 2026

pitrou commented Oct 6, 2026

View reviewed changes

taepper approved these changes Oct 6, 2026

View reviewed changes

Further reduce limit

a7c5429

pitrou requested review from assignUser, jonkeane and kou as code owners October 6, 2026 14:42

pitrou added 2 commits October 6, 2026 16:45

Rename function

bfc2f9f

Undo debug change

e71ab61

pitrou mentioned this pull request Oct 6, 2026

[C++] Excessive stack consumption with ARROW_ASSIGN_OR_RAISE #51834

Open

github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 6, 2026

		// An upper bound on the expected nesting depth of a geospatial JSON metadata object
		constexpr int kMaxJsonDepth = 20;

		// 100'000 would definitely trigger a stack overflow, validate that the error is
		// detected before that would happen.

Conversation

pitrou commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

pitrou commented Oct 5, 2026

Uh oh!

pitrou commented Oct 5, 2026

Uh oh!

pitrou commented Oct 5, 2026

Uh oh!

pitrou commented Oct 5, 2026

Uh oh!

This comment was marked as outdated.

pitrou commented Oct 5, 2026

Uh oh!

pitrou commented Oct 5, 2026

Uh oh!

Uh oh!

Uh oh!

Reranko05 commented Oct 6, 2026

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

taepper left a comment

Choose a reason for hiding this comment

Uh oh!

taepper Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

taepper Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

pitrou Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

paleolimbot Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

taepper Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

pitrou commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pitrou commented Oct 5, 2026 •

edited

Loading

taepper Oct 6, 2026 •

edited

Loading