-
Notifications
You must be signed in to change notification settings - Fork 18k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
database/sql: panic in database/sql.(*connRequestSet).deleteIndex
#68949
Comments
The current implementation has a panic when the database is closed concurrently with a new connection attempt. `database/sql.(*connRequestSet).CloseAndRemoveAll` sets `connRequestSet.s` to a `nil` slice. If this happens at the line indicated below, we get a panic: ```go db.mu.Lock() delHandle := db.connRequests.Add(req) db.mu.Unlock() // CloseAndRemoveAll here causes a panic. db.mu.Lock() deleted := db.connRequests.Delete(delHandle) db.mu.Unlock() ``` Fixes golang#68949
Change https://go.dev/cl/607238 mentions this issue: |
Update: I managed to reproduce the panic very reliably in the go playground using only exported methods https://go.dev/play/p/fHZFq6zgAV4 |
The current implementation has a panic when the database is closed concurrently with a new connection attempt. connRequestSet.CloseAndRemoveAll sets connRequestSet.s to a nil slice. If this happens between calls to connRequestSet.Add and connRequestSet.Delete, there is a panic when trying to write to the nil slice. This is sequence is likely to occur in DB.conn, where the mutex is released between calls to db.connRequests.Add and db.connRequests.Delete This change updates connRequestSet.CloseAndRemoveAll to set the curIdx to -1 for all pending requests before setting its internal slice to nil. CloseAndRemoveAll already iterates the full slice to close all the request channels. It seems appropriate to set curIdx to -1 before deleting the slice for 3 reasons: 1. connRequestSet.deleteIndex also sets curIdx to -1 2. curIdx will not be relevant to anything after the slice is set to nil 3. connRequestSet.Delete already checks for negative indices Fixes golang#68949
@cherrymui, this probably warrants a backport to the 1.23 branch. |
@gopherbot please backport this to Go 1.23. This is a regression in 1.23. Thanks. |
Backport issue(s) opened: #69041 (for 1.23). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
Change https://go.dev/cl/609255 mentions this issue: |
…and Close The current implementation has a panic when the database is closed concurrently with a new connection attempt. connRequestSet.CloseAndRemoveAll sets connRequestSet.s to a nil slice. If this happens between calls to connRequestSet.Add and connRequestSet.Delete, there is a panic when trying to write to the nil slice. This is sequence is likely to occur in DB.conn, where the mutex is released between calls to db.connRequests.Add and db.connRequests.Delete This change updates connRequestSet.CloseAndRemoveAll to set the curIdx to -1 for all pending requests before setting its internal slice to nil. CloseAndRemoveAll already iterates the full slice to close all the request channels. It seems appropriate to set curIdx to -1 before deleting the slice for 3 reasons: 1. connRequestSet.deleteIndex also sets curIdx to -1 2. curIdx will not be relevant to anything after the slice is set to nil 3. connRequestSet.Delete already checks for negative indices For #68949 Fixes #69041 Change-Id: I6b7ebc5a71b67322908271d13865fa12f2469b87 GitHub-Last-Rev: 7d26691 GitHub-Pull-Request: #68953 Reviewed-on: https://go-review.googlesource.com/c/go/+/607238 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Ian Lance Taylor <iant@golang.org> Commit-Queue: Ian Lance Taylor <iant@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> (cherry picked from commit 08707d6) Reviewed-on: https://go-review.googlesource.com/c/go/+/609255
Go version
go version go1.23.0 darwin/arm64
Output of
go env
in your module/workspace:What did you do?
One of our unit tests seems to start a transaction concurrently with closing the database, this sometimes causes a panic here:
go/src/database/sql/sql.go
Line 3644 in a4cb37d
I haven't been able to reproduce this with
db.Conn
db.Close
calls yet, but I do have a unit test in #68953 usingdatabase/sql.(*connRequestSet).CloseAndRemoveAll
anddatabase/sql.(*connRequestSet).Delete
that reliably reproduces the panic.Update: reproduced with exported methods on
database/sql.DB
here: https://go.dev/play/p/fHZFq6zgAV4What did you see happen?
This looks like a race condition in
database/sql.(*DB).conn
:(*DB).conn
heredelHandle
is set to the result ofdb.connRequests.Add(req)
, and thendb.mu
is unlockeddb.Close
will calldb.connRequests.CloseAndRemoveAll
here(*DB).conn
re-locksdb.mu
here and callsdb.connRequests.Delete(delHandle)
delHandle.idx
is positive and no bounds checking is done before trying to index into the slice which was set tonil
by(*connRequestSet).CloseAndRemoveAll
, resulting in the panic.Here's a full stack trace (from https://github.com/gravitational/teleport/actions/runs/10427126860/job/28881284822):
What did you expect to see?
I expected no panic.
sql.DB
is advertised as "safe for concurrent use by multiple goroutines"The text was updated successfully, but these errors were encountered: