[fix](be) Backport BE fixes and refinements to branch-4.1 - #65754
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Add an explicit block check to reject null column or type pointers at operator sink/get_block boundaries, while keeping the existing type compatibility check unchanged. ### Release note None (cherry picked from commit 9816a5a)
### What problem does this PR solve? `filter_by_selector` reads from the source column and writes selected rows into a destination column, so the source column should be accessed through a const interface. Root cause: the interface was non-const, and `ColumnNullable::filter_by_selector` wrote the destination nested column through a `const_cast` on the destination nullable internals. This change makes the interface const, updates the supported column implementations, removes the nullable `const_cast`, and keeps `ColumnDictionary` read-only by using a local temporary `StringRef` buffer. ### Release note None (cherry picked from commit e23b403)
### What problem does this PR solve? Integer-to-DecimalV3 casts with zero scale previously went through the generic decimal cast path even when the target decimal precision can represent the full integer input range. For example, casting an INT column to DECIMALV3(10, 0) still used the common _from_int helper, which computes decimal scaling and range-related values that are unnecessary when the scale is zero and the cast cannot narrow the integer range. Root cause: the DecimalV3 cast implementation did not have a direct fast path for non-narrowing integer casts to zero-scale decimal types. This change adds a direct zero-scale DecimalV3 path for integer and boolean inputs when the target decimal range is not narrower than the input range. The fast path writes the input value directly into the decimal native value and preserves the existing generic path for narrowing casts, non-zero-scale casts, and overflow-sensitive cases. Local optest profiling for: select sum(cast(quantity as decimalv3(10,0))) from q14_avg_expr_100m; showed the cast expression time improving from about 119.0 ms to about 109.7 ms on 100M rows, roughly an 8% reduction in this expression counter. (cherry picked from commit 5d56602)
…he#64796) Doris converts Arrow arrays into Doris columns through `DataTypeSerDe::read_column_from_arrow`. If the Arrow producer sends malformed array metadata, such as truncated validity bitmaps, truncated offsets buffers, non-monotonic string/list offsets, or offsets pointing past the child/value buffer, the existing conversion code may read invalid Arrow memory and crash BE. Root cause: the Arrow-to-Doris serde path trusted Arrow array metadata before accessing Arrow buffers. Several hot paths call `IsNull()`, `Value()`, raw value offsets, list offsets, or child arrays directly, so malformed Arrow buffers can trigger out-of-bounds reads before Doris reports a clean error. This PR adds lightweight, type-specific Arrow input validation before those buffer accesses. The checks are modeled as local preflight checks rather than full `ValidateFull()`: validity bitmap size, fixed-width data buffer size, boolean bitmap size, binary/string offsets buffer size, per-value data range, and list/map offsets monotonicity plus child length bounds. A BE config `enable_arrow_input_validation` is added and defaults to `true`. The change also fixes an existing `FixedSizeBinaryArray` sliced-read null check: the loop uses a relative index after `GetValue(start)`, but `IsNull()` expects the original Arrow row index, so it must check `start + offset_i`. | Type | Rows | Check disabled | Check enabled | Overhead | | --- | ---: | ---: | ---: | ---: | | String | 4096 | 39,352 ns | 42,032 ns | +6.8% | | String | 65536 | 604,782 ns | 624,064 ns | +3.2% | | Int64 | 4096 | 1,480 ns | 1,523 ns | +2.9% | | Int64 | 65536 | 16,160 ns | 15,883 ns | -1.7% | | Boolean | 4096 | 4,784 ns | 4,888 ns | +2.2% | | Boolean | 65536 | 79,017 ns | 80,453 ns | +1.8% | | ArrayString | 4096 | 139,747 ns | 147,793 ns | +5.8% | | ArrayString | 65536 | 2,384,377 ns | 2,477,601 ns | +3.9% | | MapStringInt | 4096 | 84,375 ns | 96,022 ns | +13.8% | | MapStringInt | 65536 | 2,742,538 ns | 2,903,358 ns | +5.9% | None - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into --> (cherry picked from commit 43a31ac)
29dc80a to
75fb4ab
Compare
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
skip buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: the default-enabled Arrow input validation still has six independent safety or compatibility gaps.
- Missing value buffers can still be dereferenced for zero-length Binary/LargeBinary values.
- Eight readers add only the weak no-offset check and still admit invalid ranges or physical buffers.
- Extreme binary offsets overflow during signed subtraction before validation.
- LargeBinary still performs aligned typed loads from potentially unaligned external offsets.
- Valid zero-row Binary/LargeBinary arrays with omitted or zero-byte offsets are newly rejected.
- Negative signed Buffer sizes become huge size_t values and bypass the supposedly strong helpers.
The non-Arrow backports (selector constness/COW behavior, Block boundary invariants, and the DecimalV3 non-narrowing fast path) were reviewed with no additional issue. List/Map/Nullable validation, exception boundaries, config wiring, benchmark registration, and test discovery were also traced. The new tests do not cover the six reproducers below.
No existing inline comment or user-supplied focus duplicated these findings. This was a static review only because the governing review prompt prohibited local builds/tests; external CI reported the main compile/test/style/coverage checks successful. The PR became merged while this review was running, but the reviewed and current head remains 75fb4ab.
Convergence note: the third and maximum subagent round found the final Buffer-size issue. All six findings are independently verified and no candidate remains unresolved, but the mandated convergence status is incomplete because a fourth no-new-findings round was not permitted.
| arrow_validation_detail::throw_invalid_arrow(array, "data buffer size overflow"); | ||
| } | ||
| const size_t required = elem_size * count; | ||
| const size_t available = static_cast<size_t>(buffers[1]->size()); |
There was a problem hiding this comment.
[P1] Reject negative and non-addressable buffers before the size cast
Buffer::size() is signed, but this cast turns a negative declared size into a huge size_t. Arrow's public Buffer(ptr, size) constructor does not reject it, so a length-2 Int64Array backed by one value and a buffer size of -1 passes this 16-byte check and is then copied out of bounds by the Number reader. The same cast is used for validity, Boolean, binary-offset, and value buffers; a present Buffer with null CPU data also passes size-only checks. Please reject negative sizes and require addressable data whenever bytes are needed before converting to size_t, and add coverage for these buffer forms.
| inline void check_arrow_binary_offsets_buffer(const ArrowBinaryArray& array) { | ||
| check_arrow_validity_bitmap(array); | ||
| const auto& buffers = array.data()->buffers; | ||
| if (UNLIKELY(buffers.size() <= 1 || !buffers[1])) { |
There was a problem hiding this comment.
[P1] Preserve valid empty binary arrays
Arrow 17 deliberately accepts a zero-length Binary/LargeBinary array with no offsets buffer (ARROW-544), and it also permits a present zero-byte offsets buffer. This helper rejects both forms with validation enabled by default: it requires buffers[1] here and later requires one offset entry. The LargeBinary reader previously returned successfully for [0,0) because it only touched offsets inside the empty loop, so valid empty RecordBatches now fail conversion. Please special-case length() == 0 consistently with Arrow, make the 32-bit callers avoid dereferencing omitted offsets for an empty range, and add Binary/LargeBinary compatibility tests.
| Status DataTypeIPv4SerDe::read_column_from_arrow(IColumn& column, const arrow::Array* arrow_array, | ||
| int64_t start, int64_t end, | ||
| const cctz::time_zone& ctz) const { | ||
| if (config::enable_arrow_input_validation) { |
There was a problem hiding this comment.
[P1] Validate the fixed-width buffer before dereferencing it
check_arrow_no_offset() checks only length/offset metadata and rejects non-zero array offsets; it does not validate start/end or buffers[1]. A valid empty arrow::Int32Array(0, nullptr) read over [0,0) therefore passes this check, but the code below still calls buffer->data() on the null shared pointer. Non-empty null/short buffers also pass and crash or over-read. The same no-offset-only pattern remains in the changed Date/DateTimeV2/DateV2/Decimal readers (with analogous unchecked binary/struct paths). Please run the range and physical-buffer checks before the first Arrow access and cover these newly opted-in readers.
| const auto* raw_data = buffer->data() + start_offset; | ||
|
|
||
| assert_cast<ColumnType&>(column).insert_data( | ||
| reinterpret_cast<const char*>(raw_data), length); |
There was a problem hiding this comment.
[P1] Validate offsets before subtracting them
The attacker-controlled offsets are subtracted into int32_t before this check runs. With offsets {INT32_MIN, INT32_MAX}, end_offset - start_offset already causes signed-overflow UB, so the validator never gets a safe negative offset/length to reject. The number-from-string branch has the same ordering, and LargeBinary's value_length() performs the analogous int64_t subtraction. Please widen/load and validate non-negative monotonic offsets before subtracting, and add extreme-offset coverage.
| reinterpret_cast<const char*>(raw_data), length); | ||
| if (config::enable_arrow_input_validation) { | ||
| check_arrow_value_range(*concrete_array, start_offset, length, buffer_size); | ||
| } |
There was a problem hiding this comment.
[P1] Reject a missing value buffer before dereferencing it
A one-row Binary/String array with offsets {0, 0} and value_data() == nullptr still passes the new checks: buffer_size is 0 and check_arrow_value_range(..., 0, 0, 0) succeeds. This line then evaluates buffer->data() before insert_data() can ignore the pointer, so the malformed input crashes instead of producing the intended validation error. The LargeBinary branch has the same ordering. Please reject a null value buffer (or handle the zero-length value without touching it) and cover both branches with this case.
|
|
||
| for (auto offset_i = start; offset_i < end; ++offset_i) { | ||
| if (!concrete_array->IsNull(offset_i)) { | ||
| const auto* raw_data = buffer->data() + concrete_array->value_offset(offset_i); |
There was a problem hiding this comment.
[P1] Read LargeBinary offsets without aligned typed loads
These accessors load through Arrow's int64_t* raw_value_offsets_. An external offset buffer wrapped at an unaligned address (for example base + 1 with valid {0,1} offsets) passes the new size checks, then hits alignment UB here. The new list/map helper already notes that FFI/Buffer::Wrap offsets are not guaranteed aligned and uses unaligned_load for that reason. Please read and validate LargeBinary offsets through the same byte-oriented approach instead of the typed accessors.
3 participants
What problem does this PR solve?
Backport a small group of BE correctness fixes, column refinements, decimal cast optimization, and Arrow input validation to branch-4.1.
Picked changes:
filter_by_selectorconst.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)