etcd-io / etcd-io/etcd

concurrency: stale Mutex.Unlock retry can delete a newer same-session lock

Open
#22,082 2 comments 0 reactions 1 assignee Claimed by @lavacat View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.