concurrency: stale Mutex.Unlock retry can delete a newer same-session lock
- Dominant language
- Go
- Stars
- 52.3k
- Forks
- 10.5k
- Avg merge
- 2d 21h
- Merged PRs (30d)
- 43
Description
### Bug report criteria
- [x] This bug report is not security related, security issues should be disclosed privately via security@etcd.io.
- [x] This is not a support request or question, support requests or questions should be raised in the etcd [discussion forums](https://github.com/etcd-io/etcd/discussions).
- [x] I have read the etcd [bug reporting guidelines](https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/reporting_bugs.md).
- [x] Existing open issues and the etcd [frequently asked questions](https://etcd.io/docs/latest/faq/) have been checked and this is not a duplicate.
### What happened?
`concurrency.Mutex.Unlock` deletes the mutex key by key name only. If etcd
commits the delete but the client observes an error, the old `Mutex` value
retains its `myKey` and `myRev`. A later `Mutex` using the same session and
prefix reuses the same key string because the key is derived from the session
lease. The recreated key has a newer create revision. Retrying `Unlock` on the
old `Mutex` then deletes that newer key incarnation.
This violates mutex ownership: an old unlock retry can release a later
acquisition it does not own.
### What did you expect to happen?
`Unlock` should not delete a key incarnation whose create revision differs from
the revision recorded by that `Mutex`. The delete should be fenced by
`CreateRevision(m.myKey) == m.myRev`, for example through the existing
`m.IsOwner()` comparison.
### How can we reproduce it (as minimally and precisely as possible)?
Add this integration test to
`tests/integration/clientv3/concurrency/mutex_test.go` and run:
```console
cd tests
go test ./integration/clientv3/concurrency -run '^TestMutexStaleUnlockDoesNotDeleteSameSessionRelock$' -count=1
```
Add these imports to the existing import block:
```go
"context"
"sync/atomic"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
pb "go.etcd.io/etcd/api/v3/etcdserverpb"
```
```go
func TestMutexStaleUnlockDoesNotDeleteSameSessionRelock(t *testing.T) {
var loseFirstDeleteReply atomic.Bool
loseFirstDeleteReply.Store(true)
loseDeleteReply := func(
ctx context.Context,
method string,
req, reply any,
cc *grpc.ClientConn,
invoker grpc.UnaryInvoker,
opts ...grpc.CallOption,
) error {
err := invoker(ctx, method, req, reply, cc, opts...)
deletesKey := method == "/etcdserverpb.KV/DeleteRange"
if txn, ok := req.(*pb.TxnRequest); ok {
for _, op := range txn.Success {
deletesKey = deletesKey || op.GetRequestDeleteRange() != nil
}
}
if err == nil && deletesKey && loseFirstDeleteReply.CompareAndSwap(true, false) {
return status.Error(codes.DeadlineExceeded, "simulated lost delete response")
}
return err
}
cli, err := integration.NewClient(t, clientv3.Config{
Endpoints: exampleEndpoints(),
MaxUnaryRetries: 1,
DialOptions: []grpc.DialOption{
grpc.WithChainUnaryInterceptor(loseDeleteReply),
},
})
require.NoError(t, err)
defer cli.Close()
session, err := concurrency.NewSession(cli)
require.NoError(t, err)
defer session.Close()
ctx := context.Background()
oldMutex := concurrency.NewMutex(session, "/my-lock/")
require.NoError(t, oldMutex.Lock(ctx))
oldKey := oldMutex.Key()
oldGet, err := cli.Get(ctx, oldKey)
require.NoError(t, err)
require.Len(t, oldGet.Kvs, 1)
oldRevision := oldGet.Kvs[0].CreateRevision
require.Error(t, oldMutex.Unlock(ctx))
afterLostReply, err := cli.Get(ctx, oldKey)
require.NoError(t, err)
require.Empty(t, afterLostReply.Kvs)
currentMutex := concurrency.NewMutex(session, "/my-lock/")
require.NoError(t, currentMutex.Lock(ctx))
require.Equal(t, oldKey, currentMutex.Key())
currentGet, err := cli.Get(ctx, currentMutex.Key())
require.NoError(t, err)
require.Len(t, currentGet.Kvs, 1)
require.Greater(t, currentGet.Kvs[0].CreateRevision, oldRevision)
require.NoError(t, oldMutex.Unlock(ctx))
afterStaleUnlock, err := cli.Get(ctx, currentMutex.Key())
require.NoError(t, err)
require.Len(t, afterStaleUnlock.Kvs, 1)
}
```
### Anything else we need to know?
At the tested commit, [`Unlock` calls an unconditional key-only
delete](https://github.com/etcd-io/etcd/blob/12caed621b38312cc1084113415bf135c59e6457/client/v3/concurrency/mutex.go#L143-L149),
while [`IsOwner` already exposes the recorded create-revision
comparison](https://github.com/etcd-io/etcd/blob/12caed621b38312cc1084113415bf135c59e6457/client/v3/concurrency/mutex.go#L152-L154).
One possible fix is to make `Unlock` a conditional transaction:
```go
client.Txn(ctx).If(m.IsOwner()).Then(v3.OpDelete(m.myKey)).Commit()
```
The important part is that the delete is guarded by the create revision this
`Mutex` recorded when it acquired the lock. The behavior when that comparison
fails can be chosen separately; it must not delete the newer key.
### Etcd version (please run commands below)
Confirmed on:
```console
$ git rev-parse HEAD
12caed621b38312cc1084113415bf135c59e6457
$ go version
go version go1.26.5 linux/amd64
embedded integration server version: 3.8.0-alpha.0
also reproduced at v3.6.13 (b0f9ef190952e6e66a778513097a02ee41220727)
```
### Etcd configuration (command line flags or environment variables)
Single-node embedded integration cluster from the etcd test framework.
### Etcd debug information
No external cluster or separately installed `etcdctl` was used. The
reproduction builds and starts an embedded server from the source checkout.
### Relevant log output
```text
--- FAIL: TestMutexStaleUnlockDoesNotDeleteSameSessionRelock
Error: "[]" should have 1 item(s), but has 0
FAIL go.etcd.io/etcd/tests/v3/integration/clientv3/concurrency
```
Contributor guide
Assessment
This issue has not been assessed yet.