Repository navigation
fix: guard etcd watch teardown against detached-thread use-after-free - #231
Merged
Merged
Conversation
etcd-cpp-apiv3 runs each Watcher's wait callback on a detached thread that can outlive EtcdClientImpl. The callbacks captured a raw `this`, so a callback arriving after SIGTERM teardown freed the client wrote into already-released memory, corrupting the glibc heap (malloc_consolidate abort / invalid pointer passed to free). Change each wait callback to capture a shared guard (mutex + alive flag) instead of raw `this`. stop_watch() clears alive under the lock as its first step, before any later callback can touch the object, and a stale comment in stop_watch() describing the old (incorrect) teardown behavior is corrected. Add an ASan regression test (test_etcd_watch_teardown) on the controller side that overrides pthread_create to delay new watcher threads during teardown, reproducing the race deterministically: 20/20 iterations caught the use-after-free before the fix, 0/20 after. Wired into run_tests.sh and `make test`. Add node_shutdown_lib.sh for e2e: shutdown steps in phases 17/19/33/40 and the harness's final stop now verify the node log ends with "bye!" with no apport record and no kernel trap, so a crash during teardown is no longer silently treated as a pass. The verdict is a self-validated predicate (FAIL when evidence can't be read) with 4 new shutdown drills added to validate. Verified: make test 2408/2408, make all, full e2e (run with no flags) 198/198 plus 25/25 validate drills. A 50-iteration repro loop on the fixed build caught 0 crashes, versus 4 crashes in 30 iterations on master. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Summary
Watcher's wait callback on a detached thread that can outliveEtcdClientImpl. The callbacks captured a rawthis, so on SIGTERM teardown a callback arriving after the client was freed wrote into already-released memory, corrupting the glibc heap (malloc_consolidateabort / invalid pointer passed tofree).shared_ptr<WatchCallbackGuard>with a mutex +aliveflag) instead of rawthis.stop_watch()clearsaliveunder the lock as its very first step, before any later callback can touch the object. A stale comment instop_watch()describing the old (incorrect) teardown behavior is also corrected.test_etcd_watch_teardown) on the controller side that overridespthread_createto delay new watcher threads during teardown, reproducing the race deterministically: 20/20 iterations caught the use-after-free before the fix, 0/20 after. Wired intorun_tests.shandmake test.node_shutdown_lib.shfor e2e: shutdown steps in phases 17/19/33/40 and the harness's final stop now verify the node log ends withbye!, with no apport record and no kernel trap, so a crash during teardown is no longer silently treated as a pass. The verdict is a self-validated predicate (FAILs when evidence can't be read) with 4 new shutdown drills added to validate.Test plan
make test— 2408/2408 passedmake all— production build rebuilt./run_e2e_test.sh, no flags) — 198/198 passed, 25/25 validate drills caught🤖 Generated with Claude Code