924060929 opened a new pull request, #64521:
URL: https://github.com/apache/doris/pull/64521
### What problem does this PR solve?
Problem Summary:
`retention()` keeps its aggregate state in a fixed-size
`RetentionState::events[32]` array and serializes it into a single `int64`
bitmap, so it can represent at most 32 events. However neither FE nor BE
limited the number of arguments:
- `AggregateFunctionRetention::add()` iterates over all argument columns and
calls `RetentionState::set(i)` (`events[i] = 1`) without any bound check, so
with more than 32 params it writes past the end of `events[]` (heap
out-of-bounds write).
- `RetentionState::insert_result_into()` reads `events[]` for the actual
argument count, so it reads past the array as well.
With a query calling `retention()` with 102 boolean arguments, this
corrupted the heap and later crashed BE in the streaming aggregation output
path:
```
[E-3113]string column length is too large: total_length=4295003729,
element_number=4064
...
doris::vectorized::ColumnStr<unsigned int>::insert_many_strings -> memcpy
(SIGSEGV)
doris::pipeline::StreamingAggLocalState::_get_results_with_serialized_key
```
This PR enforces the 32-event limit on both sides:
- **FE** (`Retention.checkLegalityBeforeTypeCoercion`): reject `> 32` params
with a clear `AnalysisException` at planning time.
- **BE** (`AggregateFunctionRetention` constructor): throw
`INVALID_ARGUMENT` when more than `RetentionState::MAX_EVENTS` argument types
are given, as a backstop for any path that reaches BE (e.g. an older FE during
a rolling upgrade, or a direct BE call).
### Release note
Fix BE crash when `retention()` is called with more than 32 conditions; it
now reports a clear error instead of corrupting memory.
### Check List (For Author)
- Test
- [x] Regression test
- [x] 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
- Behavior changed:
- [ ] No.
- [x] Yes. `retention()` with more than 32 conditions now fails with a
clear error at planning time instead of returning wrong results or crashing BE.
- Does this need documentation?
- [x] No.
- [ ] Yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
