Skip to content

Handle JSON memory pool allocation failures - #3663

Open
wasphin wants to merge 2 commits into
apache:masterfrom
wasphin:fix-json-pool-allocation-failure
Open

wasphin wants to merge 2 commits into
apache:masterfrom
wasphin:fix-json-pool-allocation-failure

Conversation

@wasphin

@wasphin wasphin commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

What problem does this PR solve?

Problem Summary:

The JSON memory pool dereferences a failed chunk allocation and assumes a
successful allocation when growing a buffer. This PR fixes the allocator's
failure-return contract only. It does not make DOM/parser users tolerant of
allocation failures: bundled callers still dereference allocation results
without checking, so an OOM can still terminate those operations.

What is changed and the side effects?

Changed:

  • Return nullptr from pool allocation and reallocation when the backing allocator fails.
  • Preserve existing chunks and contents after a failed allocation.
  • Back-port the allocation-failure handling used by upstream RapidJSON's
    MemoryPoolAllocator, adapted to this bundled version. This keeps the allocator
    behavior aligned for future re-vendoring.
  • Add a failing backing-allocator regression to CMake and Bazel.

Upstream reference: https://github.com/Tencent/rapidjson/blob/master/include/rapidjson/allocators.h

Side effects:

  • Performance effects: Negligible checks after allocating a chunk.
  • Breaking backward compatibility: No public signature changes. The allocator
    now reports backing-allocation failures; DOM/parser OOM recovery remains
    outside this change and requires handling in their callers.

Check List:

Copilot AI left a comment

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.

🟢 Approval recommended

The localized checks preserve pool state on failure, and the regression covers failure and recovery paths; tests were not executed during this review.

No open issues 🎉

What changed in this PR

This PR makes bRPC’s bundled RapidJSON memory pool handle backing-allocation failures without dereferencing null pointers or altering existing pool contents.

Changes:

  • Return null when chunk allocation or buffer growth fails.
  • Add regression coverage for allocation failures, state preservation, and successful recovery.
  • Register the regression test in CMake and Bazel.
File Description
test/​rapidjson_allocator_unittest.cc Tests failure handling, preserved contents, and recovery.
test/​CMakeLists.txt Adds the regression to the CMake test sources.
test/​BUILD.bazel Adds the regression to the Bazel test sources.
src/​butil/​third_party/​rapidjson/​allocators.h Checks allocation results before modifying chunks or copying data.

Review effort: Balanced

@wwbmmm wwbmmm left a comment

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.

[replied by brpc-oncall robot] The allocator-level null propagation is correct and matches upstream RapidJSON, but it does not remove the null-dereference risk on the DOM/parser path because the bundled consumers never check for failure; please either complete the handling or state the limited scope explicitly, and fix the small nits below.

Comment thread test/rapidjson_allocator_unittest.cc Resolved
Comment thread test/rapidjson_allocator_unittest.cc Resolved
Document AddChunk's failure return and unchanged chunk list. Use nullptr
only in newly added failure handling, and align the regression test's name
and memcpy qualification with repository conventions.

The allocation checks back-port upstream RapidJSON allocator behavior.
DOM/parser callers still require their own failure handling; this change
does not make those callers recover from allocation failures.
@wasphin
wasphin requested a review from wwbmmm November 8, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants