[fix](be) Remove unsafe JsonbWriter key overload#63355
Open
mrhhsg wants to merge 1 commit into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
mrhhsg
force-pushed
the
codex/remove-jsonb-writer-cstr-key
branch
from
5c39e10 to
3cbbf65
Compare
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: no blocking issues found in the PR patch.
Critical checkpoint conclusions:
- Goal/test: The PR removes the unsafe JsonbWriter::writeKey(const char*) overload and updates existing tests to use explicit lengths; the changed tests cover the affected call sites in JSONB unit tests.
- Scope: The change is small and focused on the overload removal plus required test updates.
- Concurrency/lifecycle/config/compatibility: Not applicable; no runtime concurrency, lifecycle, config, storage format, or protocol changes are introduced.
- Parallel code paths: Existing C++ call sites either already pass explicit lengths/key ids or are updated in tests; no remaining single-argument char* use was found in the reviewed patch.
- Error handling/memory/data correctness: No new Status handling or memory ownership paths are introduced. The explicit-length API preserves embedded NUL bytes that the removed strlen overload could truncate.
- Tests: Relevant BE unit tests are updated; I did not rerun them in this review-only pass.
- Observability/performance: Not applicable; this is compile-time API hardening.
User focus: no additional user-provided review focus was present.
Member
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 31274 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 169646 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
reviewed
May 19, 2026
| } | ||
|
|
||
| bool writeKey(const char* key) { return writeKey(key, strlen(key)); } | ||
| bool writeKey(const char* key) = delete; |
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: JsonbWriter::writeKey(const char*) used strlen to infer the key length, which made callers vulnerable to silent truncation when passing data with embedded NUL bytes. Remove the overload so callers must use the explicit-length API.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=JsonbDocumentTest.*:JsonbSerializeTest.*:JsonbParserTest.*:ConvertFieldToTypeTest.*
- build-support/clang-format.sh
- build-support/check-format.sh
- build-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN (failed: existing clang-tidy environment/pre-existing diagnostics report missing stddef.h and unmatched NOLINTEND in be/src/core/types.h)
- Behavior changed: Yes. Internal C++ callers can no longer use JsonbWriter::writeKey(const char*) and must pass an explicit key length.
- Does this need documentation: No
mrhhsg
force-pushed
the
codex/remove-jsonb-writer-cstr-key
branch
from
3cbbf65 to
1b8c30c
Compare
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31532 ms |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: N/A
Problem Summary:
JsonbWriter::writeKey(const char*)inferred key length withstrlen, which silently truncated byte strings that contained embedded NUL bytes. Remove this overload so callers must use the explicit-length API.Release note
None
Check List (For Author)
./run-be-ut.sh --run --filter=JsonbDocumentTest.*:JsonbSerializeTest.*:JsonbParserTest.*:ConvertFieldToTypeTest.*build-support/clang-format.shbuild-support/check-format.shbuild-support/run-clang-tidy.sh --build-dir be/ut_build_ASAN(failed: existing clang-tidy environment/pre-existing diagnostics report missingstddef.hand unmatchedNOLINTENDinbe/src/core/types.h)JsonbWriter::writeKey(const char*)and must pass an explicit key length.