Skip to content

Fix out-of-bounds read in dump() for large indentation widths#5273

Open
Angadi56 wants to merge 2 commits into
nlohmann:developfrom
Angadi56:dump-indent-oob
Open

Fix out-of-bounds read in dump() for large indentation widths#5273
Angadi56 wants to merge 2 commits into
nlohmann:developfrom
Angadi56:dump-indent-oob

Conversation

@Angadi56

Copy link
Copy Markdown

When pretty-printing, the serializer keeps one reusable indent_string buffer and widens it on demand before writing a run of indentation. The width it needs is current_indent + indent_step, which grows with nesting depth and with the indent argument given to dump(). The buffer only doubled once behind an if, so when the required width was more than twice the current size the buffer stayed too short and the following write_characters(indent_string.c_str(), new_indent) ran off its end, copying whatever heap bytes followed into the output. It reproduces under AddressSanitizer with json j = {{"a", 1}}; j.dump(1100);, which reads 1100 bytes out of a 1040-byte allocation. Looping the doubling until the buffer actually holds new_indent closes it while keeping the same amortized growth. I applied it to the three pretty-print branches (object, array, binary) that share the pattern, and added a serialization test that checks the emitted indentation is the expected run of spaces at a width past the first doubling.

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>
// variable to hold indentation for recursive calls
const auto new_indent = current_indent + indent_step;
if (JSON_HEDLEY_UNLIKELY(indent_string.size() < new_indent))
while (JSON_HEDLEY_UNLIKELY(indent_string.size() < new_indent))

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.

Maybe instead go in a single step instead of growing over multiple calls.

indent_string.resize((std::max)(indent_string.size() * 2, new_indent));

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good call, that's cleaner. Switched to the single resize with (std::max)(size*2, new_indent) so it still doubles for amortized growth but jumps straight to the needed width when one doubling wouldn't cover it. Had to cast new_indent to string_t::size_type so max deduces one type. Pushed and re-amalgamated.

Signed-off-by: Angadi Yashaswini <angadi@digiscrypt.com>

Copy link
Copy Markdown
Owner

Thanks for the report and fix! This looks like a duplicate of #5186, which I opened back in May and is still open. Both fix the same underlying bug in serializer.hpp: the indent_string buffer only doubles once when growing (indent_string.resize(indent_string.size() * 2, ...)), so a large enough indent_step/nesting depth leaves the buffer too small and write_characters reads past its end. Both PRs apply essentially the same fix (resize to max(size*2, new_indent)) across the same three call sites (object, array, binary).

#5186 additionally fixes a second bug where the resize used a literal space instead of the configured indent_char.

I'll consolidate on #5186 (possibly folding in the reserve_indent() refactor and overflow assertions from this PR) and close this one as a duplicate — let me know if you'd like credit noted differently.


Generated by Claude Code

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.

3 participants