Skip to content

Prevent stack overflow in json_pointer::flatten - #5517

Open
dajiaohuang wants to merge 4 commits into
nlohmann:developfrom
dajiaohuang:fix/5393-iterative-flatten
Open

Prevent stack overflow in json_pointer::flatten#5517
dajiaohuang wants to merge 4 commits into
nlohmann:developfrom
dajiaohuang:fix/5393-iterative-flatten

Conversation

@dajiaohuang

Copy link
Copy Markdown
Contributor

Summary

Replace the recursive implementation of json_pointer::flatten() with an explicit work stack so deeply nested values no longer exhaust the call stack. Existing traversal order and empty-container handling are preserved.

This addresses the flatten() portion of #5393.

Validation

  • test-regression2_cpp11 and test-regression2_cpp20 pass, including a 100,000-level nesting regression.

  • test-json_pointer_cpp11 and test-json_pointer_cpp20 pass.

  • The generated-header single_detail_json_pointer smoke executable passes.

  • git diff --check passes.

  • The changes are described in detail, both the what and why.

  • An existing issue is referenced.

  • The source code is amalgamated.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
};

std::vector<flatten_task> stack;
stack.push_back({reference_string, &value});

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.

Should be able to use emplace_back(reference_string, &value);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented in 46d2b4d: added the C++11-compatible task constructor so the initial frame uses emplace_back(reference_string, &value). The focused C++11/C++20 regression builds pass.

for (std::size_t i = current.value->m_data.m_value.array->size(); i > 0; --i)
{
const auto index = i - 1;
stack.push_back({detail::concat<string_t>(current.reference_string, '/', std::to_string(index)),

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.

emplace_back

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented in 46d2b4d: array child frames now use emplace_back with the same traversal and pointer semantics. The focused C++11/C++20 regression builds pass.

children.reserve(current.value->m_data.m_value.object->size());
for (const auto& element : *current.value->m_data.m_value.object)
{
children.push_back({detail::concat<string_t>(current.reference_string, '/', detail::escape(element.first)),

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.

emplace_back

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Implemented in 46d2b4d: object child frames now use emplace_back; the generated single-header smoke build also passes.

flatten(detail::concat<string_t>(reference_string, '/', std::to_string(i)),
value.m_data.m_value.array->operator[](i), result);
// iterate array and use index as a reference string
for (std::size_t i = current.value->m_data.m_value.array->size(); i > 0; --i)

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.

Comment about why this is iterating backwards?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Documented in 46d2b4d: children are pushed in reverse because the work stack is LIFO, preserving the original object iteration order. The issue #5393 regression passes in C++11 and C++20.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
@github-actions

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
@dajiaohuang

Copy link
Copy Markdown
Contributor Author

Applied the amalgamation-patch artifact from run 34485452558. Commit fe2f71e updates only include/nlohmann/detail/json_pointer.hpp and single_include/nlohmann/json.hpp to match the generated formatting, and is pushed to fix/5393-iterative-flatten. Validation passed: Release C++11 and C++20 issue #5393 regression selections (1 test, 3 assertions each), Release single_detail_json_pointer build and smoke run, and git diff --check.

@dajiaohuang

Copy link
Copy Markdown
Contributor Author

The GCC C++26 standards jobs exposed a -Werror=noexcept failure for the iterative flatten stack when instantiated with the alternative string type: latten_task's move construction was inferred as potentially throwing. Commit 7121dfd marks that constructor
oexcept in both the source and amalgamated headers. git diff --check passes; the focused C++26 CI jobs will provide the hosted validation.

Signed-off-by: dajiaohuang <mikewushuwen@outlook.com>
@dajiaohuang
dajiaohuang force-pushed the fix/5393-iterative-flatten branch from 7121dfd to 0eabb21 Compare September 10, 2026 17:28
@dajiaohuang

Copy link
Copy Markdown
Contributor Author

Amended the latest fix commit with the repository-required DCO sign-off and force-updated the fork branch. New head: 0eabb21. No source changes beyond the Signed-off-by trailer; the previous C++26 noexcept fix remains intact.

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.

2 participants