Fix recursive MSan report in HandledSignals::reset on shutdown by groeneai · Pull Request #107442 · ClickHouse/ClickHouse

groeneai · 2026-06-14T16:44:00Z

Changelog category (leave one):

CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

...

Description

Related: #107425 (where this failure was triaged and the fix was requested).

Fixes a use-after-dtor of the HandledSignals singleton on shutdown, originally seen in Stress test (arm_msan) / Stress test (amd_msan) as an infinitely recursive MSan report. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=107425&sha=b2f68b66f7510cdaaec64fb58e7c5601b58180c0&name_0=PR&name_1=Stress%20test%20%28arm_msan%29

Root cause: HandledSignals::instance() was a function-local static, so the singleton was destroyed at process exit. But the object is reachable from the signal handlers and from the registered sanitizer death callback (sanitizerDeathCallback() -> instance().reset(false)), both of which can run after static destructors. After destruction MSan poisons the members ("Member fields were destroyed" is MSan's use-after-dtor origin), so reset() iterating the destroyed handled_signals vector reads poisoned memory, raises an MSan warning, which calls Die() -> the death callback -> reset() again, recursing until the original report is lost.

Fix: intentionally leak the singleton (never destroy it), the same idiom already used for DateLUT::getInstance(). Its members then outlive every signal handler and death callback, so the post-exit read is on live, valid memory and no warning (and therefore no recursion) is produced. The now-redundant destructor (which only called reset() during static teardown, the exact window this removes) is dropped; normal shutdown still resets handlers and closes the pipe via the explicit instance().reset() calls in the BaseDaemon and ClientApplicationBase destructors.

No behavior change in non-sanitizer builds.

Version info

Merged into: 26.7.1.930 (included in 26.7 and later)

groeneai · 2026-06-14T16:44:18Z

Pre-PR validation gate

#	Question	Answer
a	Deterministic repro?	The full-server MSan repro is non-deterministic (the seeding warning is chronic exit-time noise), so I reproduced the fix mechanism deterministically at codegen level instead: compiling `reset()`'s exact body with the CI MSan flags (`clang++-21 -fsanitize=memory -fsanitize-memory-use-after-dtor`) emits 4 `__msan_warning` calls without the attribute and 0 with it. That is the precise instrumentation whose firing inside `Die()`'s death callback drives the recursion.
b	Root cause explained?	At process exit the static `HandledSignals` singleton is destroyed; MSan poisons its members (report ends "Member fields were destroyed" = use-after-dtor origin). A later MSan warning calls `__sanitizer::Die()`, which runs `sanitizerDeathCallback()`. The callback is `DISABLE_SANITIZER_INSTRUMENTATION` but that attribute does not propagate to callees; it calls `HandledSignals::reset(false)`, which iterates the poisoned `handled_signals` while still instrumented -> fires another `__msan_warning` -> `Die()` -> callback -> `reset()` -> unbounded recursion (frames #16-#32 repeat at one PC), destroying the original report and turning the job into ERROR.
c	Fix matches root cause?	Yes. `reset()` is the only callee reachable from the death callback; marking it `DISABLE_SANITIZER_INSTRUMENTATION` removes exactly the instrumentation that re-enters the sanitizer. No behavior change (still resets handlers + closes pipe); macro is a no-op in non-sanitizer builds.
d	Test intent preserved / new tests added?	No existing test is modified or weakened. No new functional test is added: the failure is a sanitizer-build-only shutdown artifact with a nondeterministic seed and no SQL-level trigger, so a stable regression test is not feasible; the codegen IR check in (a) is the verification.
e	Both directions demonstrated?	Yes, at codegen level: `reset` body -> 4 `__msan_warning` calls without the attribute (recursion seed present), 0 with it (seed removed). The `-O2` inlined form (matching the single-PC stack) goes 14 -> 0 `__msan_` calls.
f	Fix is general, not a narrow patch?	The death callback (`sanitizerDeathCallback`) has exactly one callee, `HandledSignals::reset`; there is no sibling path with the same pattern. Prior art #78178 already hardened the other reentrancy hazard in the same callback (writing to a closed pipe). This addresses the root cause (instrumentation reentrancy from the death callback), not a symptom.

Session id: cron:clickhouse-worker-slot-2:20260614-161500

groeneai · 2026-06-14T16:45:07Z

cc @azat @Algunenano — could you review this? It marks HandledSignals::reset with DISABLE_SANITIZER_INSTRUMENTATION to stop an infinite MSan recursion on shutdown: the sanitizer death callback (already non-instrumented) calls reset(), which iterates the poisoned handled_signals while still instrumented, re-triggering Die(). Same callback hardened earlier in #78178.

clickhouse-gh · 2026-06-14T17:57:57Z

Workflow [PR], commit [e602d07]

Summary: ❌

Performance Comparison: Performance dashboard

job_name	test_name	status	info
Integration tests (amd_asan_ubsan, db disk, old analyzer, 3/6)		FAIL
	test_replicated_database/test.py::test_replicated_table_structure_alter	FAIL	cidb, issue
Integration tests (amd_msan, 7/8)		FAIL
	test_replicated_database/test.py::test_replicated_table_structure_alter	FAIL	cidb, issue

AI Review

Summary

The current head routes the sanitizer death callback through resetHandledSignals, which becomes a no-op once normal shutdown has already reset the handlers, instead of touching a potentially destroyed HandledSignals singleton during exit. I reviewed the current diff, the full SignalHandlers implementation, the prior discussion, and the current CI report, and I did not find a remaining correctness, lifetime, or teardown issue in the updated fix.

Final Verdict

Status: ✅ Approve

groeneai · 2026-06-14T19:01:28Z

Pre-PR validation gate (revised fix)

#	Question	Answer
a	Deterministic repro?	The failure is a sanitizer-only, nondeterministic shutdown artifact (exit-time death-callback reentrancy, no SQL trigger), so a stable on-demand functional repro is not feasible. Instead the exact mechanism is reproduced deterministically at the IR level: compiling `instance()` with the CI MSan flags (`-fsanitize=memory -fsanitize-memory-use-after-dtor`) shows the Meyers-singleton form schedules a destructor (`__cxa_atexit`) and poisons members on destruction (`__sanitizer_dtor_callback`), which is the use-after-dtor seed read by `reset()`.
b	Root cause explained?	`HandledSignals::instance()` was a function-local static, destroyed at process exit. The object is reachable from signal handlers and the registered sanitizer death callback (`sanitizerDeathCallback -> instance().reset(false)`), both of which can run after static destructors. After destruction MSan poisons the members; `reset()` iterating the destroyed `handled_signals` vector reads poisoned memory, fires an MSan warning, which calls `Die()` -> the death callback -> `reset()` again, recursing until the original report is lost and the job becomes ERROR.
c	Fix matches root cause?	Yes. The bad state is a destroyed-then-read singleton. The fix makes the singleton never-destroyed (intentionally leaked), so the read is on live, valid memory rather than freed/poisoned storage. This addresses the source (invalid access), not just the symptom (the MSan report).
d	Test intent preserved / new tests added?	No existing test is modified or weakened. No new functional test is added: a sanitizer-only, nondeterministic, no-SQL-trigger shutdown artifact has no stable functional regression test; the IR-level structural check (no `__cxa_atexit` / no dtor poisoning for the leaked instance) is the verification.
e	Both directions demonstrated?	Yes, at the IR level with the CI MSan flags: OLD (Meyers) emits `__cxa_atexit=2` and `__sanitizer_dtor_callback=2` (the seed present); NEW (leaked) emits `__cxa_atexit=0` and `__sanitizer_dtor_callback=0` (seed removed). The modified TU also compiles clean in the debug build (RC=0).
f	Fix is general, not a narrow patch?	Yes. The leak also closes the related window where signal handlers (which write to `instance().signal_pipe` at lines 75/101/126/213) could touch a destroyed `PipeFDs` if a signal arrived during/after static teardown, not only the `reset()` path the bot flagged. All other `instance().reset()` callers (`BaseDaemon`, `ClientApplicationBase`, disks, gtest) are unaffected and still perform explicit cleanup.

Session id: cron:clickhouse-worker-slot-3:20260614-184900

groeneai · 2026-06-14T23:56:27Z

Pre-PR validation gate (updated for head `fe7e8f8`)

#	Question	Answer
a	Deterministic repro?	Yes. `clickhouse local -q "SELECT 1"` under MSan aborts on every exit (exit 134). Confirmed on the `amd_msan` binary for the prior head `f057d41` and reproduced locally.
b	Root cause explained?	Two layers. (1) Original: the Meyers-singleton `HandledSignals` is destroyed at process exit, but it is reachable from signal handlers + the sanitizer death callback (which run after static dtors); `reset()` then iterates the destroyed `handled_signals` vector -> MSan warning -> `Die()` -> death callback -> `reset()` -> recursion. (2) The first revision kept `DISABLE_SANITIZER_INSTRUMENTATION` on `reset()`; an uninstrumented `reset()` never writes "defined" shadow for its range-loop iterator locals, so they inherit stale poison from the just-destroyed locals in the preceding `writeSignalIDtoSignalPipe()` call, and the instrumented `operator==` callee reports it -> deterministic abort on the normal shutdown path (`~ClientApplicationBase:67`).
c	Fix matches root cause?	Yes. Leak the singleton so members are never destroyed/poisoned (fixes both the recursion and the destroyed-state access at the source), and remove `DISABLE_SANITIZER_INSTRUMENTATION` (the cause of layer 2; unnecessary once nothing is poisoned). Not a suppression.
d	Test intent preserved / new tests added?	No assertions weakened. No dedicated regression test added: the artifact is a sanitizer-only, nondeterministic shutdown report with no SQL trigger, so a stable functional test is not feasible; the `amd_msan` job itself (every `client`/`local` invocation exits cleanly) is the regression guard, and the integration suite exercises it thousands of times per run.
e	Both directions demonstrated?	Yes, with `amd_msan` binaries built from source. OLD (`f057d41`): `clickhouse local -q "SELECT 1"` -> exit 134, 36 use-of-uninit warnings. NEW (`fe7e8f8`): exit 0, 0 warnings, `HandledSignals::reset` absent from all stacks. Also clean: `numbers(1000)`, `version()`, `client --help`. `MSAN_OPTIONS=poison_in_dtor=0` makes OLD pass, confirming the use-after-dtor origin.
f	Fix is general, not a narrow patch?	Yes. The fix is at the source (object lifetime), not a guard at the crash site. The only other callers of `HandledSignals::instance().reset()` (`~BaseDaemon`, `~ClientApplicationBase`) now also operate on the live, never-destroyed object, so the same class of post-teardown read is fixed for all of them rather than one path. No sibling implementations exist.

Session id: cron:clickhouse-worker-slot-0:20260614-225000

`test_implicit_index_upgrade_alter_replay` creates a second replica `node2` and asserts `SELECT count() FROM test_alter_replay` equals `10001` immediately after `wait_for_active_replica`. But `wait_for_active_replica` only waits for `is_readonly = 0`, not for the freshly-joined replica to finish fetching parts. So `node2` can observe only the small `VALUES` part (1 row, `key = 99999`) before the 10000-row part is fetched, and the assertion fails with `assert '1' == '10001'`. This raced once each on several unrelated PRs over the last 30 days (e.g. ClickHouse#108084, ClickHouse#99280, ClickHouse#107566, ClickHouse#107442). Add `SYSTEM SYNC REPLICA test_alter_replay` after the replica becomes active, which blocks until all parts are fetched, before checking the row count. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108770&sha=bb760616d54e3d8e5d20968022085506c31b5f37&name_0=PR&name_1=Integration%20tests%20%28arm_binary%2C%20distributed%20plan%2C%201%2F4%29 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

azat

I don't think this is a good idea, now in sanitizer death callback it will create "HandledSignals" object which will create files, but likely we cannot run such syscalls from death callback (since we avoid "close" there), so let's instead introduce "resetHandledSignals()" that will reset signal handlers back (like "reset(false)") but will not do anything if they are already reseted back, for this introduce static variable

groeneai · 2026-07-13T11:58:50Z

Done, implemented as requested (ddc4a2a).

New free function resetHandledSignals(), guarded by a file-scope static std::atomic_flag handled_signals_were_reset. It is a no-op once the handlers were already reset.
HandledSignals::reset() sets the flag first, before the reset loop.
The death callback now calls resetHandledSignals() instead of HandledSignals::instance().reset(false).

So after the normal-shutdown reset, the death-callback path does nothing: it never re-enters reset(), never touches HandledSignals state that may be destroyed at exit, and never constructs the singleton (no pipe open / file creation from the death callback). Setting the flag before the loop also makes any re-entry return immediately, which is what broke the original recursion. The singleton stays a plain function-local static, destroyed at exit as before.

Verified on a from-source MSan build (-fsanitize=memory -fsanitize-memory-use-after-dtor): clickhouse local -q "SELECT 1" and a server start + SELECT 1 + SIGTERM shutdown both exit 0 with zero MSan warnings and no HandledSignals::reset in any stack.

groeneai · 2026-07-13T11:59:11Z

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. On a from-source MSan build (`-fsanitize=memory -fsanitize-memory-use-after-dtor`) the pre-fix code produced use-of-uninitialized-value on exit (`HandledSignals::reset` recursing through the death callback). Reproduced via `clickhouse local -q "SELECT 1"` and server start + SIGTERM.
b	Root cause explained?	The death callback ran `HandledSignals::instance().reset(false)`. At process exit the singleton is destroyed, so `reset()` iterated the destroyed `handled_signals` vector; under MSan-use-after-dtor that read raised a warning, whose death callback called `reset()` again, recursing until the original report was lost.
c	Fix matches root cause?	Yes. The death-callback path is made idempotent via `resetHandledSignals()` + a static flag set by `reset()`. After normal shutdown reset it is a no-op: no re-entry into `reset()`, no read of possibly-destroyed state, no singleton construction (so no pipe open / file creation from the death callback, per the reviewer).
d	Test intent preserved / new tests added?	No behavior change to normal signal handling / shutdown. A sanitizer-only, nondeterministic exit-time artifact with no SQL trigger has no stable functional regression test; verification is the from-source MSan run below plus the full CI MSan matrix.
e	Both directions demonstrated?	Pre-fix: MSan warning + `HandledSignals::reset` recursion on exit. Post-fix (`ddc4a2a`): `SELECT 1` and server start + SIGTERM both exit 0, zero MSan warnings, `HandledSignals::reset` absent from all stacks.
f	Fix is general across code paths?	The death callback is the only path that resets while the singleton may be destroyed; it is the single site routed through the guarded `resetHandledSignals()`. The explicit `instance().reset()` calls in `~BaseDaemon` / `~ClientApplicationBase` run before teardown and are unaffected.
g	Fix generalizes across inputs?	N/A (process-exit signal-handler teardown; no data inputs / types / parameters involved).
h	Backward compatible?	Yes. No settings, formats, or user-visible behavior changed. Normal shutdown still resets handlers exactly as before.
i	Invariants and contracts preserved?	Yes. The death callback still avoids `close`/blocking syscalls (now also avoids singleton construction). `reset()` sets the flag before its loop, so the recursion-break holds even on re-entry; the singleton remains a plain function-local static destroyed at exit.

Session id: cron:clickhouse-worker-slot-4:20260713-102400

azat

Ok, but the comments are too verbose, please do not document all "thoughts" that you have in mind while developing, at max this can be in the commit description. Comments in the code should include only essential details. Fix and rebase.

The HandledSignals singleton is reachable from the sanitizer death callback, which can run after static destructors at process exit. The callback called HandledSignals::instance().reset(false); under MSan-use-after-dtor the read of the destroyed handled_signals vector fired a use-of-uninitialized-value warning, whose death callback called reset() again, recursing until the original report was lost. Do not leak/reconstruct the singleton from the death callback: constructing HandledSignals there opens a pipe and creates files, syscalls we must avoid in a death callback (that is why the callback does not close the pipe either). Instead reset from a free function resetHandledSignals() guarded by a static flag. HandledSignals::reset() sets the flag before touching any state, so once the handlers were reset during normal shutdown the death-callback path is a no-op: it never re-enters reset(), never reads HandledSignals state that may be destroyed at exit, and never constructs the singleton. The flag being set before the reset loop also makes any re-entry return immediately, breaking the recursion. The singleton stays a normal function-local static (destroyed at exit as before); only the death-callback path is made idempotent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

groeneai · 2026-07-13T19:45:54Z

Trimmed the code comments to essentials (rationale is in the commit description) and rebased on master.

clickhouse-gh · 2026-07-14T01:35:15Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.80%	85.80%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	78.00%	78.00%	+0.00%

Changed lines: Changed C/C++ lines covered: 5/8 (62.50%) · Uncovered code

Full report · Diff report

groeneai · 2026-07-14T03:17:23Z

CI finish ledger — `e602d07`

Every failure below has an owner: a fixing PR, or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Integration tests (amd_asan_ubsan db disk old analyzer; amd_msan; amd_tsan) / test_replicated_database::test_replicated_table_structure_alter	flaky (chronic trunk, 206 PRs / 40 master in 30d)	#110030 (ours, open)
Stateless (various) / 02346_text_index_bug108519_qcc_skip_index	flaky (materialize_statistics_on_insert, 128 PRs / 57 master in 30d)	#110260 (ours, open)
Stateless (amd_llvm_coverage, old analyzer, s3, DBReplicated, WasmEdge) / Server died + Unknown error	crash (trunk server-died: Int256*Float64 multiply LOGICAL_ERROR during partial-result header eval; concurrent EXCHANGE TABLES); lane passed on retry this head	#109747 (ours, open)
Stateless (amd_tsan, s3 storage, sequential) / Scraping system tables	infra (minio_* dump timeout via clickhouse-local)	#110099 (ours, open)

This PR is a shutdown signal-handler MSan fix (HandledSignals::reset). None of the failures are in a code path this PR touches. No PR-caused failure.

Session id: cron:clickhouse-worker-slot-2:20260714-032100

groeneai mentioned this pull request Jun 14, 2026

Honor materialized_postgresql_schema in MaterializedPostgreSQL table engine #107425

Open

1 task

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jun 14, 2026

clickhouse-gh Bot added the pr-ci label Jun 14, 2026

clickhouse-gh Bot reviewed Jun 14, 2026

View reviewed changes

Comment thread src/Common/SignalHandlers.cpp Outdated

groeneai force-pushed the fix-msan-recursion-handledsignals-reset branch from 36f9f92 to f057d41 Compare June 14, 2026 19:00

groeneai force-pushed the fix-msan-recursion-handledsignals-reset branch from f057d41 to fe7e8f8 Compare June 14, 2026 23:54

This was referenced Jun 15, 2026

Feature: Paimon minmax index #100160

Open

Randomize tests with DETACH/ATTACH table before query execution #96130

Open

This was referenced Jun 24, 2026

Fix data-type names containing the substring INT being parsed as MySQL integers #108354

Merged

Fix flaky test_dns_cache: use default start timeout in cluster_ready #108498

Open

alexey-milovidov mentioned this pull request Jun 29, 2026

Add as-you-type autocompletion (inline hints) to the CLI #108070

Merged

1 task

This was referenced Jun 30, 2026

Reduce flakiness of test_distributed_ddl_parallel and test_restore_replica #108933

Merged

Backport #108493 to 25.8: Fix crash on deeply nested expressions with raised max_parser_depth #108896

Merged

This was referenced Jun 30, 2026

Implement CREATE HANDLER: SQL-defined HTTP handlers #106231

Open

Fix flaky test_implicit_index_upgrade_alter_replay (replica fetch race) #108961

Merged

Fix hang in SYSTEM SYNC MERGES with the Manual merge selector #108770

Merged

groeneai mentioned this pull request Jun 30, 2026

Clear ColumnReplicated insertion cache on rollback #108935

Merged

alexey-milovidov mentioned this pull request Jun 30, 2026

Parallelize read-in-order from a single part with PrefetchingConcatProcessor #100391

Open

groeneai mentioned this pull request Jun 30, 2026

Fix flaky test_backup_restore_on_cluster restore timeout #108993

Merged

azat requested changes Jul 13, 2026

View reviewed changes

azat self-assigned this Jul 13, 2026

groeneai force-pushed the fix-msan-recursion-handledsignals-reset branch from fe7e8f8 to ddc4a2a Compare July 13, 2026 11:58

azat reviewed Jul 13, 2026

View reviewed changes

groeneai force-pushed the fix-msan-recursion-handledsignals-reset branch from ddc4a2a to 729490d Compare July 13, 2026 19:45

Merge branch 'master' into fix-msan-recursion-handledsignals-reset

e602d07

azat approved these changes Jul 13, 2026

View reviewed changes

azat enabled auto-merge July 13, 2026 20:01

azat added this pull request to the merge queue Jul 14, 2026

Merged via the queue into ClickHouse:master with commit f4bd25f Jul 14, 2026
173 of 176 checks passed

robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 14, 2026

robot-clickhouse mentioned this pull request Jul 14, 2026

Revert "#107442: Fix recursive MSan report in HandledSignals::reset on shutdown" #110442

Open

Uh oh!

Conversation

groeneai commented Jun 14, 2026 • edited by robot-clickhouse-ci-1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Description

Version info

Uh oh!

groeneai commented Jun 14, 2026

Pre-PR validation gate

Uh oh!

groeneai commented Jun 14, 2026

Uh oh!

clickhouse-gh Bot commented Jun 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Final Verdict

Uh oh!

Uh oh!

groeneai commented Jun 14, 2026

Pre-PR validation gate (revised fix)

Uh oh!

groeneai commented Jun 14, 2026

Pre-PR validation gate (updated for head fe7e8f8)

Uh oh!

azat left a comment

Choose a reason for hiding this comment

Uh oh!

groeneai commented Jul 13, 2026

Uh oh!

groeneai commented Jul 13, 2026

Uh oh!

azat left a comment

Choose a reason for hiding this comment

Uh oh!

groeneai commented Jul 13, 2026

Uh oh!

clickhouse-gh Bot commented Jul 14, 2026

LLVM Coverage Report

Uh oh!

groeneai commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI finish ledger — e602d07

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

groeneai commented Jun 14, 2026 •

edited by robot-clickhouse-ci-1

Loading

clickhouse-gh Bot commented Jun 14, 2026 •

edited

Loading

Pre-PR validation gate (updated for head `fe7e8f8`)

groeneai commented Jul 14, 2026 •

edited

Loading

CI finish ledger — `e602d07`