Skip to content

database/sql: panic in database/sql.(*connRequestSet).deleteIndex [1.23 backport] #69041

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

Closed
gopherbot opened this issue Aug 23, 2024 · 2 comments
Labels
CherryPickApproved Used during the release process for point releases
Milestone

Comments

@gopherbot
Copy link
Contributor

@cherrymui requested issue #68949 to be considered for backport to the next 1.23 minor release.

@gopherbot please backport this to Go 1.23. This is a regression in 1.23. Thanks.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Aug 23, 2024
@gopherbot gopherbot added this to the Go1.23.1 milestone Aug 23, 2024
@prattmic prattmic added the CherryPickApproved Used during the release process for point releases label Aug 28, 2024
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Aug 28, 2024
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/609255 mentions this issue: [release-branch.go1.23] database/sql: fix panic with concurrent Conn and Close

gopherbot pushed a commit that referenced this issue Aug 28, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…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
@gopherbot
Copy link
Contributor Author

Closed by merging CL 609255 (commit 6de5a71) to release-branch.go1.23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPickApproved Used during the release process for point releases
Projects
None yet
Development

No branches or pull requests

2 participants