[fix](be) Fix auto_partition_name crash for invalid runtime arguments - #67218
Open
felixwluo wants to merge 3 commits into
Open
[fix](be) Fix auto_partition_name crash for invalid runtime arguments#67218felixwluo wants to merge 3 commits into
felixwluo wants to merge 3 commits into
Conversation
… segments in the rowset reader (apache#35484) ## Proposed changes pick apache#35432 ## Further comments If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Member
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.
Request changes.
Two blocking correctness issues are detailed inline: the function-wide constant override breaks the public constant-expression contract, and the nullable unwrapping leaves a materialized null map without an owner.
Critical checkpoint conclusions:
- Goal and correctness: The new arity/control checks prevent the reported short-RANGE and invalid-dispatch out-of-bounds paths, and the
ColumnString::get_data_at()rewrite preserves valid output bytes. The implementation is still unsafe because of the two inline blockers. - Tests: The added cases cover the numeric LIST reproducer and the new RANGE arity guard. They do not cover no-fold nesting/physical constness, and their
Constedinputs construct the const-nullable ownership shape affected by the use-after-free. Focused no-fold and ASAN const-nullable coverage is required. - Scope and user focus: The PR changes only the BE implementation and its unit test; no additional user focus was supplied. The global constant-expression behavior and removal of the null-map lifetime keeper have effects broader than the intended validation hardening.
- Control flow and parallel paths: LIST and RANGE dispatch, literal validation, all six RANGE granularities, const/vector/nullable/empty inputs, Unicode and long-name hashing, and date/datetime splitting were checked. No additional PR-introduced defect survived validation. The pre-existing RANGE-null payload behavior was excluded from this review.
- Concurrency and lifecycle: No concurrency mechanism changes. Lifecycle/ownership is blocking at the second inline comment because later validation and row loops dereference the released null map.
- Configuration and compatibility: No configuration, protocol, storage-format, or rolling-upgrade surface changed. The new BE argument checks align with the current FE legality rules.
- Error handling and observability: Invalid new call shapes return a non-OK
Statusthrough the existing expression path. Existing errors are sufficient; no new logging or metrics are needed. - Persistence, transactions, and data writes: Not applicable to these scalar-expression changes.
- Performance: The first blocker also changes literal-only evaluation from cached once to repeated work and allocation per input row when folding is skipped.
- Validation status: Review-only run; no builds or tests were executed. The authoritative patch and relevant callers, wrappers, column ownership code, FE checks, and test harness were inspected. Round 2 converged with all three reviewers reporting no new valuable findings, so the review is complete with these two blockers.
| size_t get_number_of_arguments() const override { return 0; } | ||
| bool is_variadic() const override { return true; } | ||
| bool use_default_implementation_for_nulls() const override { return false; } | ||
| bool use_default_implementation_for_constants() const override { return false; } |
Contributor
There was a problem hiding this comment.
[P1] Preserve the constant-expression contract here
This override is also what VectorizedFnCall::is_constant() consults, so it does more than keep the control columns wrapped during execute_impl(). With folding skipped, auto_partition_name('list', 'x') now produces an ordinary ColumnString instead of a cached ColumnConst. For example, SELECT /*+SET_VAR(debug_skip_fold_constant=true)*/ trim('abc', auto_partition_name('list', 'x')) reaches FunctionTrim::get_arguments_that_are_always_constant() == {1} and is rejected by the generic constant-argument check, although the same deterministic nested expression was constant before this change. Literal-only projections also recompute and allocate the name once per input row (load planning is one path that explicitly skips folding). Please preserve VectorizedFnCall constness while validating the original control arguments through FunctionContext constant-column metadata or another decoupled mechanism, and add a no-fold nested regression.
| @@ -122,40 +125,49 @@ class FunctionAutoPartitionName : public IFunction { | |||
| if (const auto* nullable = | |||
| check_and_get_column<const ColumnNullable>(*argument_columns[i])) { | |||
| null_list[i] = &nullable->get_null_map_data(); | |||
Contributor
There was a problem hiding this comment.
[P1] Keep the materialized null map alive
For a ColumnConst(ColumnNullable(...)), convert_to_full_column_if_const() creates a new full ColumnNullable with a freshly allocated null map. null_list[i] keeps only a raw pointer into that map, and the next assignment retains the nested string but releases the sole owner of the nullable parent and its null map. The later checks at lines 151/223/275 therefore read freed storage. The deleted argument_null_columns[i] = nullable->get_null_map_column_ptr() was the lifetime guard; the changed Consted tests build this exact const-nullable shape, as do supported LIST calls with NULL values. Please retain either the full materialized nullable column or its null-map ColumnPtr for the duration of execution, and cover both NULL and non-NULL const-nullable inputs under ASAN.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17030 ms |
Contributor
TPC-DS: Total hot run time: 81897 ms |
Contributor
ClickBench: Total hot run time: 14.54 s |
Contributor
TPC-H: Total hot run time: 17075 ms |
Contributor
TPC-DS: Total hot run time: 82293 ms |
Contributor
ClickBench: Total hot run time: 14.82 s |
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?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Executing
select auto_partition_name('list', '10')could cause BE to core dump. The BE implementation ofauto_partition_namerelied on FE-side validation and did not validate runtime argument count or control arguments before dispatching. When the partition type was not handled aslist, the code fell through to therangepath and unconditionally accessed the third argument. For two-argument LIST calls, this could trigger an out-of-bounds vector access and abort the BE process.This change fixes the issue at the BE function implementation layer by validating the argument count and partition type before dispatching, validating RANGE granularity explicitly, and reading string arguments through
ColumnString::get_data_at()instead of manual chars/offsets indexing. It also disables the default constant-argument handling for this function so the function can preserve and validate its own constant control arguments. A BE unit test coversAUTO_PARTITION_NAME('LIST', '10')and malformed RANGE arguments.Release note
Fix a BE crash when
auto_partition_namereceives invalid or mis-dispatched runtime arguments.None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)