[fix](be) Bound BitmapValue::deserialize to prevent heap over-read on tampered BITMAP payloads#63702
Open
jacktengg wants to merge 2 commits into
Open
[fix](be) Bound BitmapValue::deserialize to prevent heap over-read on tampered BITMAP payloads#63702jacktengg wants to merge 2 commits into
jacktengg wants to merge 2 commits into
Conversation
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.
I found blocking issues in the error propagation around the new bounded bitmap deserialization. The core bound checks are directionally correct, and the added unit/regression cases cover the main malicious payload shapes, but several migrated call sites still ignore false from BitmapValue::deserialize, so malformed bitmap payloads can be accepted as default or partially initialized values instead of failing.
Critical checkpoint conclusions:
- Goal/test coverage: The PR aims to bound bitmap deserialization and adds relevant BE/regression tests, but the goal is not fully achieved because not all migrated call sites fail on decode errors.
- Scope/clarity: The change is focused on bitmap deserialization hardening.
- Concurrency/lifecycle: No new concurrency or special lifecycle concerns found.
- Configuration/compatibility: No new config or wire-format/storage-format change; existing format is read with extra bounds.
- Parallel paths: Some parallel deserialization paths were updated but still miss return-value handling, as noted inline.
- Error handling: Blocking issue: decode failures are silently ignored in some paths, violating Error Means Failure.
- Data correctness: Blocking issue: malformed serialized bitmap data can become an empty/partial bitmap and produce incorrect results.
- Memory/performance/observability: No additional blocking concerns found beyond potential noisy ERROR logs on rejected user input.
- User focus: No additional user-provided review focus was present.
| data_ptr += unaligned_load<size_t>(&meta_ptr[i]); | ||
| size_t one_size = unaligned_load<size_t>(&meta_ptr[i]); | ||
| data[i].deserialize(data_ptr, one_size); | ||
| data_ptr += one_size; |
Contributor
There was a problem hiding this comment.
BitmapValue::deserialize now returns false for truncated or malformed input, but this block deserialization path still ignores that result. If a serialized block carries a bad bitmap payload, data[i] remains default/partially initialized and the reader advances by the declared size, so execution can continue with wrong bitmap values instead of failing. Please check the return value here and raise/propagate an error; the same applies to deserialize_as_stream below, which currently ignores value.deserialize(ref.data, ref.size) as well.
| } | ||
|
|
||
| if constexpr (T == TYPE_BITMAP) { | ||
| pvalue->deserialize(pos); |
Contributor
There was a problem hiding this comment.
This newly bounded decode can fail, but insert_binary_data has already inserted a default value and then ignores the false return. A malformed bitmap binary cell will therefore stay in the column as an empty/default bitmap rather than making the load/read fail, which violates the hardening goal and can silently corrupt query results. Please check the return value and throw/propagate an error before leaving the inserted value in place.
Contributor
TPC-H: Total hot run time: 31507 ms |
Contributor
TPC-DS: Total hot run time: 171898 ms |
… tampered BITMAP payloads
Problem Summary:
`BitmapValue::deserialize(const char* src)` lacked a buffer length and
called upstream `Roaring64Map::read(src)` whose documentation explicitly
warns it is unsafe: "if you provide bad data, many bytes could be read,
possibly causing a buffer overflow". The function trusts an inline
`map_size` varint (unbounded) and the per-bucket `roaring::Roaring::read`
(also unsafe, no `maxbytes`). A crafted/corrupt BITMAP payload, e.g. one
fed through `bitmap_from_base64('BP////8P')` (BITMAP64 + varint
UINT32_MAX), would loop deep past the end of the input buffer.
The pre-existing `try/catch(std::runtime_error)` only catches roaring's
own exceptions; a silent over-read does not necessarily throw and ASAN
flags it as heap-buffer-overflow.
Fix:
- Add `Roaring64Map::readSafe(buf, maxbytes)` built on the CRoaring
`roaring_bitmap_*deserialize_safe` primitives, with an explicit upper
bound on the outer `map_size` varint for BITMAP64.
- Add a bounded `BitmapValue::deserialize(const char* src, size_t maxbytes)`
that validates every per-branch size (SINGLE32/SINGLE64, BITMAP32/64,
v2 portable, SET, SET_V2) before reading and catches both
`std::runtime_error` and `doris::Exception`.
- Replace the unsafe `BitmapValue(const char*)` constructor with
`BitmapValue(const char*, size_t maxbytes)` that throws on failure.
- Migrate all untrusted callers (`data_type_bitmap_serde.cpp`,
`function_bitmap.cpp` / `bitmap_from_base64`, `data_type_bitmap.cpp`,
`column_complex.h`) to pass the actual buffer length.
- Harden `BitmapIntersect<T>::deserialize` similarly: `Helper::read_from`
now bounds-checks every read (POD, datetime, decimal, string), and
`BitmapIntersect::deserialize` and its constructor take an explicit
`maxbytes`. Update the single caller in
`aggregate_function_orthogonal_bitmap.h`.
- Drop the unused `BitmapExprCalculation(const char*)` constructor.
- Test:
- Regression test: added 6 `bitmap_from_base64` cases in
`test_bitmap_function.groovy` covering BITMAP64 / BITMAP64_V2 / SET_V2
oversized-size payloads and three invalid type-code payloads. All
expect the new decode-failure exception.
- Unit Test: added 5 BE cases in `bitmap_value_test.cpp`
(`deserialize_malicious_bitmap64_map_size`,
`deserialize_malicious_bitmap64v2_map_size`,
`deserialize_truncated_single`,
`deserialize_malicious_set_v2`,
`deserialize_bounded_roundtrip`). Ran
`./run-be-ut.sh --run --filter='BitmapValueTest.*'` — 34/34 pass under
ASAN.
jacktengg
force-pushed
the
260526-bitmap-improve
branch
from
0425da0 to
e776293
Compare
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.
Problem Summary:
BitmapValue::deserialize(const char* src)lacked a buffer length and called upstreamRoaring64Map::read(src)whose documentation explicitly warns it is unsafe: "if you provide bad data, many bytes could be read, possibly causing a buffer overflow". The function trusts an inlinemap_sizevarint (unbounded) and the per-bucketroaring::Roaring::read(also unsafe, nomaxbytes). A crafted/corrupt BITMAP payload, e.g. one fed throughbitmap_from_base64('BP////8P')(BITMAP64 + varint UINT32_MAX), would loop deep past the end of the input buffer.The pre-existing
try/catch(std::runtime_error)only catches roaring's own exceptions; a silent over-read does not necessarily throw and ASAN flags it as heap-buffer-overflow.Fix:
Roaring64Map::readSafe(buf, maxbytes)built on the CRoaringroaring_bitmap_*deserialize_safeprimitives, with an explicit upper bound on the outermap_sizevarint for BITMAP64.BitmapValue::deserialize(const char* src, size_t maxbytes)that validates every per-branch size (SINGLE32/SINGLE64, BITMAP32/64, v2 portable, SET, SET_V2) before reading and catches bothstd::runtime_erroranddoris::Exception.BitmapValue(const char*)constructor withBitmapValue(const char*, size_t maxbytes)that throws on failure.data_type_bitmap_serde.cpp,function_bitmap.cpp/bitmap_from_base64,data_type_bitmap.cpp,column_complex.h) to pass the actual buffer length.BitmapIntersect<T>::deserializesimilarly:Helper::read_fromnow bounds-checks every read (POD, datetime, decimal, string), andBitmapIntersect::deserializeand its constructor take an explicitmaxbytes. Update the single caller inaggregate_function_orthogonal_bitmap.h.BitmapExprCalculation(const char*)constructor.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)