[fix](be) Avoid UB from unaligned __int128 dereference#63703
Open
jacktengg wants to merge 1 commit into
Open
Conversation
Issue Number: close #xxx Problem Summary: Several BE call sites obtained a byte pointer from StringRef::data / Slice::data / a generic const void* (e.g. ORC pushdown literal value, JSONB serde, runtime filter literal builder, meta_tool column dump) and dereferenced it as `__int128*` / `int128_t*` / `DecimalV2Value*` / `Decimal<int128_t>*`. Because those buffers carry no 16-byte alignment guarantee, the load is undefined behavior. On alignment-strict targets (some aarch64 / SPARC builds) and under UBSan -fsanitize=alignment the read can SIGBUS, abort, or - with SSE codegen for __int128 - fault on a movdqa instruction. Sites fixed: - be/src/core/data_type_serde/data_type_number_serde.cpp (LARGEINT JSONB) - be/src/format/orc/vorc_reader.cpp (TYPE_DECIMALV2 / TYPE_DECIMAL128I literal conversion for ORC predicate push-down) - be/src/tools/meta_tool.cpp (LARGEINT and DECIMAL128I dump) - be/src/exprs/vexpr.h create_texpr_literal_node<>: TYPE_LARGEINT, TYPE_DECIMALV2 and TYPE_DECIMAL128I literal construction All these sites now load the 16-byte value through the `unaligned_load<T>` helper from `util/unaligned.h` into a local __int128 / DecimalV2Value / Decimal<int128_t> before use. Modern compilers reduce the helper's memcpy to a load, so there is no measurable performance impact, but the semantics become well-defined regardless of the producer's alignment. Note: be/src/runtime/fold_constant_executor.cpp also contains unaligned __int128 reads for TYPE_LARGEINT and TYPE_DECIMALV2 in `_get_result`, but that branch is unreachable under the current Nereids planner (which always sets `is_nereids = true` and uses `be_exec_version >= 4`, taking the protobuf serde path). It is left untouched here to keep the diff focused; the dead branch can be cleaned up separately.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: no blocking issues found.
Critical checkpoint conclusions:
- Goal: The PR fixes undefined behavior from reading 16-byte integer/decimal payloads through potentially unaligned byte pointers in JSONB serialization, vexpr literal construction, ORC predicate literal conversion, and meta_tool display. The changed code uses the existing
unaligned_load<T>helper, which is appropriate for these trivially-copyable value types. - Scope/focus: The code change is small and focused on the affected dereferences; no unrelated behavior changes were found in the GitHub PR diff.
- Concurrency: The touched code does not introduce shared mutable state, locks, or new thread/bthread lifecycle concerns.
- Lifecycle/static initialization: No new global/static lifecycle dependency was introduced.
- Configuration: No configuration items were added.
- Compatibility: No storage format, RPC, thrift/protobuf schema, or function-symbol compatibility change was introduced; values are loaded differently but serialized/formatted as before.
- Parallel paths: The PR updates the directly targeted 16-byte load sites. I also checked for remaining similar
__int128/DecimalV2Valuereinterpret dereferences; the knownFoldConstantExecutor::_get_resultlegacy path remains, but based on the current Nereids caller it is gated away bybe_exec_version >= 4 && is_nereidsand is called out in the PR description. - Conditional checks: No new special runtime condition was added.
- Tests: A BE unit test deliberately passes misaligned buffers through
create_texpr_literal_nodefor LARGEINT, DECIMALV2, and DECIMAL128I, and it is picked up by the BE test recursive glob. The regression test follows Doris regression standards (order_qt, drop-before-use, hardcoded table name, generated.out). One non-blocking caveat: the SQL regression mostly exercises normal column/runtime paths and does not itself guarantee unaligned storage for every fixed call site, so the BE unit test is the strongest direct proof of the unaligned-buffer contract. - Test results: The added
.outis deterministic and consistent with the queries. - Observability: No new observability is needed for this low-level UB fix.
- Transactions/persistence/data writes: No transaction, persistence, version visibility, delete bitmap, or data-write semantics are changed.
- FE/BE variables: No new FE-BE variable passing is involved.
- Performance: The replacement is a small
memcpy-based unaligned load that compilers generally lower efficiently; no meaningful hot-path regression was found. - User focus: No additional user-provided review focus was present.
Residual risk: I did not run the full BE unit/regression suite in this review environment; this conclusion is based on static review of the PR diff and surrounding code.
Contributor
TPC-H: Total hot run time: 31960 ms |
Contributor
TPC-DS: Total hot run time: 171975 ms |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31404 ms |
Contributor
TPC-DS: Total hot run time: 171537 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
2 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.
Issue Number: close #xxx
Problem Summary: Several BE call sites obtained a byte pointer from StringRef::data / Slice::data / a generic const void* (e.g. ORC pushdown literal value, JSONB serde, runtime filter literal builder, meta_tool column dump) and dereferenced it as
__int128*/int128_t*/DecimalV2Value*/Decimal<int128_t>*.Because those buffers carry no 16-byte alignment guarantee, the load is undefined behavior. On alignment-strict targets (some aarch64 / SPARC builds) and under UBSan -fsanitize=alignment the read can SIGBUS, abort, or - with SSE codegen for __int128 - fault on a movdqa instruction.
Sites fixed:
All these sites now load the 16-byte value through the
unaligned_load<T>helper fromutil/unaligned.hinto a local __int128 / DecimalV2Value / Decimal<int128_t> before use. Modern compilers reduce the helper's memcpy to a load, so there is no measurable performance impact, but the semantics become well-defined regardless of the producer's alignment.Note: be/src/runtime/fold_constant_executor.cpp also contains unaligned __int128 reads for TYPE_LARGEINT and TYPE_DECIMALV2 in
_get_result, but that branch is unreachable under the current Nereids planner (which always setsis_nereids = trueand usesbe_exec_version >= 4, taking the protobuf serde path). It is left untouched here to keep the diff focused; the dead branch can be cleaned up separately.What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)