Fix reading one element past the end of the parsed sequenceMatch pattern - #119871
Fix reading one element past the end of the parsed sequenceMatch pattern#119871groeneai wants to merge 1 commit into
Conversation
The terminal block of AggregateFunctionSequenceBase::backtrackingMatch
bound-checked action_it != action_end on entry only, then advanced action_it
with no re-check. When every remaining action is satisfied by a zero-length
match (KleeneStar, TimeLessOrEqual, TimeLess, or TimeGreaterOrEqual with
extra == 0), the loop walks action_it to action_end and the condition
dereferences it, reading the never-initialised tail of the inline
PODArrayWithStackMemory buffer that holds actions.
MemorySanitizer reported it as use-of-uninitialized-value at
AggregateFunctionSequenceMatch.cpp:555:69, from sequenceMatchEvents, which has
no conditions_met early-out and so reaches the block even with an empty event
list. Beyond the out-of-bounds read, the byte found there decides whether the
loop continues, so the function's return value is decided by uninitialised
memory. Writing a KleeneStar action at actions[size()] on an unfixed build
makes sequenceMatch('(?t<=10)') return 0 instead of 1, and
sequenceCount('(?1).*') return 0 instead of 1, on the inputs the new test uses.
Folding the entry if into the while gives one guard covering both ->type and
->extra. Behaviour changes only where the loop used to run past action_end,
i.e. only where the old answer was decided by uninitialised memory.
The missing check dates to 184e6f8 (2023-11-06). ASan never caught it
because the read lands inside a live allocation, so it is not a
heap-buffer-overflow; only MSan tracks initialisedness.
The first of the four new assertions is a reachability case, not a value
assertion: for an empty event list sequenceMatchEvents returns [] whether or
not the read happens, so under MSan the sanitizer report is its oracle. The
other three assert values that the unfixed code gets wrong when the byte past
the end happens to match.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
groeneai
added
can be tested
Internal second-model review: adjudication log (click to expand)Independent pre-publication review (engine: codex): 0 findings. My own cold review of the resulting The nits, all settled outside the diff: the relationship link was inside the template's HTML comment Session id: cron:clickhouse-review-slot-9:20260914-000040 |
|
Workflow [PR], commit [4517ae8] Summary: ❌
AI ReviewSummaryThis PR fixes the terminal zero-length suffix handling in Findings
Final VerdictChanges requested. The matcher fix looks sound, but the regression coverage should prove the sanitizer bug in a PR-gated job instead of relying on nondeterministic stateless behavior. LLVM Coverage ReportMeasured on commit 4517ae8.
Changed lines: Changed C/C++ lines covered: 8/9 (88.89%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| select [] = sequenceMatchEvents('')(t, c = 1, c = 2) from values('t UInt32, c UInt8', (0, 0), (1, 0)); | ||
| select 1 = sequenceMatch('(?t<=10)')(t, c = 1, c = 2) from values('t UInt32, c UInt8', (0, 0), (1, 0)); | ||
| -- The events can also run out part way through the pattern, leaving a trailing `.*`. | ||
| select 1 = sequenceCount('(?1).*')(t, c = 1, c = 2) from values('t UInt32, c UInt8', (0, 1)); |
There was a problem hiding this comment.
The fix in backtrackingMatch looks right, but this regression file still does not fail reliably on the unfixed code in the jobs that gate the PR. Line 4 is only an MSan oracle, and PR CI does not run 0_stateless under MSan; lines 5 and 7 still depend on whatever byte happens to live in actions[size()] on the broken build, so they can go green there as well.
That leaves the actual contract of this PR, "the terminal zero-length path no longer reads uninitialized memory", without deterministic coverage before merge. Please add a reproducer that fails before the fix in a PR-gated suite, or exercise this path in a unit_tests_msan-style test instead of relying on normal stateless execution.
There was a problem hiding this comment.
The load-bearing premise is measurably false for this PR: 0_stateless does run under MemorySanitizer in PR CI, and it ran this exact file. Stateless tests (amd_msan, flaky check) executed it 50 times on 4517ae8a against the MSan build (that ParamSet requires CH_AMD_MSAN):
SELECT check_name, test_status, count() FROM default.checks
WHERE commit_sha = '4517ae8a65416bf7661151a3b33c9905a7c25ad0'
AND test_name LIKE '%05212%' AND check_name LIKE '%msan%' GROUP BY 1, 2Stateless tests (amd_msan, flaky check) OK 50
Failure before the fix is gated mechanically too. Bugfix validation (functional tests, amd64/aarch64), scheduled here by the pr-bugfix label, runs the added test against master-HEAD binaries whose build set includes amd_msan and arm_msan (ci/jobs/scripts/bugfix_validation.py:9-10) and inverts the verdict (ci/jobs/functional_tests.py:285): the job passes only if the added test FAILS on the unfixed binary. That is exactly the property you ask for. Its copy from the first workflow dispatch was cancelled when the can be tested label re-triggered CI, so it still owes a verdict on this head. After merge the file also lands in Stateless tests (amd_msan, parallel|sequential), which runs the full suite (4737 distinct tests in shard 1/3 last week).
On the substance you are right, and the PR body says so: assertion 1 is a reachability case whose oracle is the sanitizer report, and the value assertions cannot be deterministic on a non-sanitizer build. That is a property of the defect rather than of the test. The past-end read lands inside a live allocation, which is why ASan missed it from 2023-11-06 until now and only initializedness tracking sees it. I measured both directions on a local MSan build: unfixed reproduces the full CI frame chain at AggregateFunctionSequenceMatch.cpp:555:69, fixed is clean.
I am not adding a unit_tests_msan gtest: it would detect the same read by the same mechanism with no determinism gained, because the detector is the sanitizer and not the harness. Duplicating an SQL test in C++ also runs against the standing preference here, stated by @ rschu1ze on #108878: "For future PRs, let's ask Groene to write SQL-based tests instead of C++ unit tests. Unit tests are much harder to maintain in the long run." If a reviewer prefers the gtest form anyway, say so and I will add it.
clickhouse-gh
Bot
added
the
comp-aggregate-functions
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
The official master build is compiled with Compile time of recompiled translation units7 translation units recompiled, 16 s compile time in total, 7 of them have a recent master baseline. |
CI finish ledger - 4517ae8Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only
Nine of the ten Session id: cron:our-pr-ci-monitor:20260914-050101 |
2 participants
Related: #118497
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed an out-of-bounds read of the parsed pattern in
sequenceMatch,sequenceCountandsequenceMatchEvents. When the trailing part of a pattern consisted only of actions that a zero-length match satisfies (.*and(?t...)conditions), the matcher read one element past the end of its action list, so its answer was decided by uninitialised memory andsequenceMatch/sequenceCountcould return0for a pattern that does match.Description
backtrackingMatchends with a loop consuming the pattern actions a zero-length match satisfies (KleeneStar,TimeLessOrEqual,TimeLess,TimeGreaterOrEqualwithextra == 0). It was bound-checked on entry only and advancedaction_itwith no re-check, so when every remaining action is one of those it walks toaction_endand the condition dereferences it.actionsis aPODArrayWithStackMemory<PatternAction, 64>andPatternAction::typehas no default initialiser, so the read is of uninitialised memory. The byte found there also decides whether the loop continues: writing aKleeneStaratactions[size()]on an unfixed build makessequenceMatch('(?t<=10)')andsequenceCount('(?1).*')return0instead of1.I fold the entry
ifinto thewhile, one guard covering both->typeand->extra. Behaviour changes only where the loop used to run pastaction_end.The missing check dates to 184e6f8 (2023-11-06), and reverting my own 6e6cbb2 still reproduces it. ASan never caught it because the read lands inside a live allocation; only MemorySanitizer tracks initialisedness.
No issue exists. Provenance is two
Stress test (*_msan)reports on unrelated PRs:Reports: arm_msan and amd_msan.
Local MemorySanitizer reproduction, both directions
-DCMAKE_BUILD_TYPE=None -DSANITIZE=memory,clickhouse local, no table:Without the fix this reports
use-of-uninitialized-valuewith the CI stack, symbolized:The same
:555:69report also comes fromsequenceMatch(insertResultInto:717:84) andsequenceCount(count:811:53), and from patterns ending in each of the four action kinds, withUInt8/UInt32/UInt64/DateTime/Datetimestamps. With the fix all of them are clean and return theexpected values, and
00222_sequence_aggregate_function_familyoutput is unchanged (78 lines). The newtest's first assertion is a reachability case rather than a value assertion:
sequenceMatchEvents('')returns
[]either way, so under MSan the sanitizer report is its oracle.Workflow [PR]
Sync PR [sync-upstream/pr/119871]