etcdserver: don't read storage version from a closed backend#22190
etcdserver: don't read storage version from a closed backend#22190jojinkb wants to merge 1 commit into
Conversation
When a member is being removed (or is otherwise shutting down), the run
goroutine's defer closes s.stopping and then calls Cleanup(), which
closes the backend without any synchronization against in-flight
readers. A concurrent Maintenance Status RPC that reaches
EtcdServer.StorageVersion() in that window calls
schema.DetectSchemaVersion() on the closed backend, whose read
transaction has already been reset to nil, and crashes the process:
SIGSEGV in bbolt.(*Tx).Bucket
<- backend.(*baseReadTx).UnsafeRange
<- schema.UnsafeReadStorageVersion
<- schema.DetectSchemaVersion
<- EtcdServer.StorageVersion
<- maintenanceServer.Status
Close the backend while holding the bemu lock in Cleanup(), and check
s.stopping under the bemu read lock in StorageVersion(). Since stopping
is closed strictly before Cleanup() runs, a reader that does not
observe stopping as closed is guaranteed the backend cannot be closed
while it holds the read lock, and a reader that acquires the lock after
the backend was closed observes stopping as closed and returns nil,
which the Status handler already tolerates by omitting the
storageVersion field.
Signed-off-by: Jojin <jojin.kb@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jojinkb The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jojinkb. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Fixes #22189
What this PR does
Fixes a SIGSEGV when a
Maintenance/StatusgRPC call races with server shutdown (e.g. the member being removed from the cluster).The run goroutine's shutdown defer closes
s.stoppingand then callsCleanup(), which closes the backend without synchronizing with in-flight readers. A concurrentStatushandler that reachesEtcdServer.StorageVersion()in that window callsschema.DetectSchemaVersion()on the closed backend, whose shared read transaction has already been reset tonilby the finalCommitAndStop, andbackend.(*baseReadTx).UnsafeRange()dereferences the nil*bolt.Tx:How it is fixed
Two small changes in
server/etcdserver/server.go:Cleanup()now closes the backend while holdings.bemu(the mutexStorageVersionalready takes for reading, becauseapplySnapshotcan swap the backend instance).StorageVersion()checkss.stoppingafter acquirings.bemu.RLock()and returns nil when the server is stopping.Since
s.stoppingis closed strictly beforeCleanup()runs, the lock gives the needed happens-before edge: a reader that does not observestoppingas closed cannot have its backend closed while it holds the read lock, and a reader that acquires the lock after the close observesstoppingas closed and returns nil. TheStatushandler already handles a nil storage version by omitting the field, and the internal caller (monitorStorageVersion) exits ons.stoppingbeforeCleanup()runs, so no behavior changes outside the shutdown window.Relationship to #21966 / #22188
This crash was discovered while verifying the fix for #21966: on current
mainit is masked in theStatuspath because the same handler panics earlier inRaftCluster.IsLocalMemberLearner. With #22188 applied, the #21966 reproducer immediately surfaces this second, independent crash. This change is standalone and does not depend on #22188.Testing
TestStorageVersionDuringStop(fails with the SIGSEGV stack above before the fix; the concurrent subtest also deadlocks before the fix because the panic escapesUnsafeRangewithtxMuheld):go test ./etcdserver/ -run TestStorageVersionDuringStop -v -race— PASSgo test ./etcdserver/ -count=1— okIsLocalMemberLearnerwhenMaintenance/Statusis called concurrently with member removal #21966 reproducer against a build including membership: don't panic in IsLocalMemberLearner when local member is absent #22188: before this fix, SIGSEGV on the first run; after, 3/3 clean exits with code 0 (~300k Status calls per run).This change was developed with AI assistance (Claude Code); I have reviewed and tested it.