From 317a3c130e3b59171b3bd90e7a469fba76582444 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 27 Jun 2025 17:21:20 -0400 Subject: [PATCH 1/7] [release-branch.go1.23] runtime: stash allpSnapshot on the M findRunnable takes a snapshot of allp prior to dropping the P because afterwards procresize may mutate allp without synchronization. procresize is careful to never mutate the contents up to cap(allp), so findRunnable can still safely access the Ps in the slice. Unfortunately, growing allp is problematic. If procresize grows the allp backing array, it drops the reference to the old array. allpSnapshot still refers to the old array, but allpSnapshot is on the system stack in findRunnable, which also likely no longer has a P at all. This means that a future GC will not find the reference and can free the array and use it for another allocation. This would corrupt later reads that findRunnable does from the array. The fix is simple: the M struct itself is reachable by the GC, so we can stash the snapshot in the M to ensure it is visible to the GC. The ugliest part of the CL is the cleanup when we are done with the snapshot because there are so many return/goto top sites. I am tempted to put mp.clearAllpSnapshot() in the caller and at top to make this less error prone, at the expensive of extra unnecessary writes. For #74414. Fixes #74415. Change-Id: I6a6a636c484e4f4b34794fd07910b3fffeca830b Reviewed-on: https://go-review.googlesource.com/c/go/+/684460 Reviewed-by: Cherry Mui LUCI-TryBot-Result: Go LUCI Auto-Submit: Michael Pratt (cherry picked from commit 740857f529ce4074c7f9aa1d6f38db8c4a00246c) Reviewed-on: https://go-review.googlesource.com/c/go/+/685075 --- src/runtime/proc.go | 41 ++++++++++++++++++++++++++++++++++++++++- src/runtime/runtime2.go | 1 + 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index d922dd6193989f..dfe298cc970716 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -1000,6 +1000,28 @@ func (mp *m) becomeSpinning() { sched.needspinning.Store(0) } +// Take a snapshot of allp, for use after dropping the P. +// +// Must be called with a P, but the returned slice may be used after dropping +// the P. The M holds a reference on the snapshot to keep the backing array +// alive. +// +//go:yeswritebarrierrec +func (mp *m) snapshotAllp() []*p { + mp.allpSnapshot = allp + return mp.allpSnapshot +} + +// Clear the saved allp snapshot. Should be called as soon as the snapshot is +// no longer required. +// +// Must be called after reacquiring a P, as it requires a write barrier. +// +//go:yeswritebarrierrec +func (mp *m) clearAllpSnapshot() { + mp.allpSnapshot = nil +} + func (mp *m) hasCgoOnStack() bool { return mp.ncgo > 0 || mp.isextra } @@ -3273,6 +3295,11 @@ func findRunnable() (gp *g, inheritTime, tryWakeP bool) { // an M. top: + // We may have collected an allp snapshot below. The snapshot is only + // required in each loop iteration. Clear it to all GC to collect the + // slice. + mp.clearAllpSnapshot() + pp := mp.p.ptr() if sched.gcwaiting.Load() { gcstopm() @@ -3441,7 +3468,11 @@ top: // which can change underfoot once we no longer block // safe-points. We don't need to snapshot the contents because // everything up to cap(allp) is immutable. - allpSnapshot := allp + // + // We clear the snapshot from the M after return via + // mp.clearAllpSnapshop (in schedule) and on each iteration of the top + // loop. + allpSnapshot := mp.snapshotAllp() // Also snapshot masks. Value changes are OK, but we can't allow // len to change out from under us. idlepMaskSnapshot := idlepMask @@ -3573,6 +3604,9 @@ top: pollUntil = checkTimersNoP(allpSnapshot, timerpMaskSnapshot, pollUntil) } + // We don't need allp anymore at this pointer, but can't clear the + // snapshot without a P for the write barrier.. + // Poll network until next timer. if netpollinited() && (netpollAnyWaiters() || pollUntil != 0) && sched.lastpoll.Swap(0) != 0 { sched.pollUntil.Store(pollUntil) @@ -4013,6 +4047,11 @@ top: gp, inheritTime, tryWakeP := findRunnable() // blocks until work is available + // findRunnable may have collected an allp snapshot. The snapshot is + // only required within findRunnable. Clear it to all GC to collect the + // slice. + mp.clearAllpSnapshot() + if debug.dontfreezetheworld > 0 && freezing.Load() { // See comment in freezetheworld. We don't want to perturb // scheduler state, so we didn't gcstopm in findRunnable, but diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index c88f2b70585d2c..63dfc46b00105a 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -586,6 +586,7 @@ type m struct { needextram bool g0StackAccurate bool // whether the g0 stack has accurate bounds traceback uint8 + allpSnapshot []*p // Snapshot of allp for use after dropping P in findRunnable, nil otherwise. ncgocall uint64 // number of cgo calls in total ncgo int32 // number of cgo calls currently in progress cgoCallersUse atomic.Uint32 // if non-zero, cgoCallers in use temporarily From cea5be7739cb572204e4f41dd19f8bbbfc634034 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Thu, 31 Oct 2024 20:41:51 +0000 Subject: [PATCH 2/7] [release-branch.go1.23] cmd/cgo/internal/testsanitizers: disable ASLR for TSAN tests Ever since we had to upgrade from our COS image, we've been experiencing TSAN test failures. My best guess is that the ASLR randomization entropy increased, causing TSAN to fail. TSAN already re-execs itself in Clang 18+ with ASLR disabled, so just execute the tests with ASLR disabled on Linux. For #59418. Fixes #74726. Change-Id: Icb4536ddf0f2f5e7850734564d40f5a208ab8d01 Cq-Include-Trybots: luci.golang.try:go1.23-linux-386,go1.23-linux-386-clang15,go1.23-linux-amd64-clang15,go1.23-linux-amd64-boringcrypto,go1.23-linux-amd64-goamd64v3 Reviewed-on: https://go-review.googlesource.com/c/go/+/623956 Reviewed-by: Ian Lance Taylor Reviewed-by: Keith Randall LUCI-TryBot-Result: Go LUCI (cherry picked from commit b813e6fd73e0925ca57f5b3ff6b0d991bb2e5aea) Reviewed-on: https://go-review.googlesource.com/c/go/+/690035 Reviewed-by: Michael Knyszek Reviewed-by: Dmitri Shuralyov --- src/cmd/cgo/internal/testsanitizers/cshared_test.go | 12 +++++++++++- src/cmd/cgo/internal/testsanitizers/tsan_test.go | 12 +++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/cmd/cgo/internal/testsanitizers/cshared_test.go b/src/cmd/cgo/internal/testsanitizers/cshared_test.go index f26c50a6219a51..15409d0fca04fd 100644 --- a/src/cmd/cgo/internal/testsanitizers/cshared_test.go +++ b/src/cmd/cgo/internal/testsanitizers/cshared_test.go @@ -11,6 +11,7 @@ import ( "internal/platform" "internal/testenv" "os" + "os/exec" "strings" "testing" ) @@ -90,7 +91,16 @@ func TestShared(t *testing.T) { cmd.Args = append(cmd.Args, "-o", dstBin, cSrc, lib) mustRun(t, cmd) - cmd = hangProneCmd(dstBin) + cmdArgs := []string{dstBin} + if tc.sanitizer == "thread" && GOOS == "linux" { + // Disable ASLR for TSAN. See #59418. + arch, err := exec.Command("uname", "-m").Output() + if err != nil { + t.Fatalf("failed to run `uname -m`: %v", err) + } + cmdArgs = []string{"setarch", strings.TrimSpace(string(arch)), "-R", dstBin} + } + cmd = hangProneCmd(cmdArgs[0], cmdArgs[1:]...) replaceEnv(cmd, "LD_LIBRARY_PATH", ".") mustRun(t, cmd) }) diff --git a/src/cmd/cgo/internal/testsanitizers/tsan_test.go b/src/cmd/cgo/internal/testsanitizers/tsan_test.go index 94c00ef7f4804b..74acde57f23d53 100644 --- a/src/cmd/cgo/internal/testsanitizers/tsan_test.go +++ b/src/cmd/cgo/internal/testsanitizers/tsan_test.go @@ -8,6 +8,7 @@ package sanitizers_test import ( "internal/testenv" + "os/exec" "strings" "testing" ) @@ -68,7 +69,16 @@ func TestTSAN(t *testing.T) { outPath := dir.Join(name) mustRun(t, config.goCmd("build", "-o", outPath, srcPath(tc.src))) - cmd := hangProneCmd(outPath) + cmdArgs := []string{outPath} + if goos == "linux" { + // Disable ASLR. See #59418. + arch, err := exec.Command("uname", "-m").Output() + if err != nil { + t.Fatalf("failed to run `uname -m`: %v", err) + } + cmdArgs = []string{"setarch", strings.TrimSpace(string(arch)), "-R", outPath} + } + cmd := hangProneCmd(cmdArgs[0], cmdArgs[1:]...) if tc.needsRuntime { config.skipIfRuntimeIncompatible(t) } From 6c9c80b61198430eedfd02c98d4f5baf8abd085e Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Wed, 23 Jul 2025 14:01:42 -0400 Subject: [PATCH 3/7] [release-branch.go1.23] cmd/go: skip known TestScript/build_trimpath_cgo failure on darwin/arm64 The failure is understood. Skip the test since it's failing without producing new information. For #73922. Fixes #74721. Change-Id: I339882e1a9772255b185a33daf8dee97f764f830 Reviewed-on: https://go-review.googlesource.com/c/go/+/689917 TryBot-Bypass: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov Reviewed-by: Michael Matloob Reviewed-by: Michael Knyszek --- src/cmd/go/testdata/script/build_trimpath_cgo.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/src/cmd/go/testdata/script/build_trimpath_cgo.txt b/src/cmd/go/testdata/script/build_trimpath_cgo.txt index 528982442d2c9f..cc96aa8fd635f3 100644 --- a/src/cmd/go/testdata/script/build_trimpath_cgo.txt +++ b/src/cmd/go/testdata/script/build_trimpath_cgo.txt @@ -10,6 +10,7 @@ # Check that the source path appears when -trimpath is not used. go build -o hello.exe . grep -q gopath[/\\]src hello.exe +[GOOS:darwin] [GOARCH:arm64] stop 'on darwin/arm64, the rest of this test can''t work without a Go 1.24+ fix for a debug/macho problem; see go.dev/issue/73922' go run ./list-dwarf hello.exe stdout gopath[/\\]src From e8794e650e05fad07a33fb6e3266a9e677d13fa8 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Mon, 21 Jul 2025 10:09:35 -0700 Subject: [PATCH 4/7] [release-branch.go1.23] cmd/compile: for arm64 epilog, do SP increment with a single instruction That way, the frame is atomically popped. Previously, for big frames the SP was unwound in two steps (because arm64 can only add constants up to 1<<12 in a single instruction). Fixes #74693 Change-Id: I382c249194ad7bc9fc19607c27487c58d90d49e5 Reviewed-on: https://go-review.googlesource.com/c/go/+/689235 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Pratt Reviewed-by: Keith Randall (cherry picked from commit f7cc61e7d7f77521e073137c6045ba73f66ef902) Reviewed-on: https://go-review.googlesource.com/c/go/+/689595 --- src/cmd/internal/obj/arm64/obj7.go | 55 +++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/src/cmd/internal/obj/arm64/obj7.go b/src/cmd/internal/obj/arm64/obj7.go index 20498bc2c67da7..f15b6a9bfdc7ee 100644 --- a/src/cmd/internal/obj/arm64/obj7.go +++ b/src/cmd/internal/obj/arm64/obj7.go @@ -882,18 +882,49 @@ func preprocess(ctxt *obj.Link, cursym *obj.LSym, newprog obj.ProgAlloc) { p.To.Reg = REGFP p.To.Offset = REGLINK - // ADD $aoffset, RSP, RSP - q = newprog() - q.As = AADD - q.From.Type = obj.TYPE_CONST - q.From.Offset = int64(aoffset) - q.To.Type = obj.TYPE_REG - q.To.Reg = REGSP - q.Spadj = -aoffset - q.Pos = p.Pos - q.Link = p.Link - p.Link = q - p = q + if aoffset < 1<<12 { + // ADD $aoffset, RSP, RSP + q = newprog() + q.As = AADD + q.From.Type = obj.TYPE_CONST + q.From.Offset = int64(aoffset) + q.To.Type = obj.TYPE_REG + q.To.Reg = REGSP + q.Spadj = -aoffset + q.Pos = p.Pos + q.Link = p.Link + p.Link = q + p = q + } else { + // Put frame size in a separate register and + // add it in with a single instruction, + // so we never have a partial frame during + // the epilog. See issue 73259. + + // MOVD $aoffset, REGTMP + q = newprog() + q.As = AMOVD + q.From.Type = obj.TYPE_CONST + q.From.Offset = int64(aoffset) + q.To.Type = obj.TYPE_REG + q.To.Reg = REGTMP + q.Pos = p.Pos + q.Link = p.Link + p.Link = q + p = q + // ADD REGTMP, RSP, RSP + q = newprog() + q.As = AADD + q.From.Type = obj.TYPE_REG + q.From.Reg = REGTMP + q.To.Type = obj.TYPE_REG + q.To.Reg = REGSP + q.Spadj = -aoffset + q.Pos = p.Pos + q.Link = p.Link + p.Link = q + p = q + } } // If enabled, this code emits 'MOV PC, R27' before every 'MOV LR, PC', From 8fa31a2d7d9e60c50a3a94080c097b6e65773f4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20Mengu=C3=A9?= Date: Mon, 30 Jun 2025 16:58:59 +0200 Subject: [PATCH 5/7] [release-branch.go1.23] os/exec: fix incorrect expansion of "", "." and ".." in LookPath Fix incorrect expansion of "" and "." when $PATH contains an executable file or, on Windows, a parent directory of a %PATH% element contains an file with the same name as the %PATH% element but with one of the %PATHEXT% extension (ex: C:\utils\bin is in PATH, and C:\utils\bin.exe exists). Fix incorrect expansion of ".." when $PATH contains an element which is an the concatenation of the path to an executable file (or on Windows a path that can be expanded to an executable by appending a %PATHEXT% extension), a path separator and a name. "", "." and ".." are now rejected early with ErrNotFound. Fixes CVE-2025-47906 Fixes #74803 Change-Id: Ie50cc0a660fce8fbdc952a7f2e05c36062dcb50e Reviewed-on: https://go-review.googlesource.com/c/go/+/685755 LUCI-TryBot-Result: Go LUCI Auto-Submit: Damien Neil Reviewed-by: Roland Shoemaker Reviewed-by: Damien Neil (cherry picked from commit e0b07dc22eaab1b003d98ad6d63cdfacc76c5c70) Reviewed-on: https://go-review.googlesource.com/c/go/+/691855 Reviewed-by: Michael Knyszek --- src/os/exec/dot_test.go | 56 +++++++++++++++++++++++++++++++++++++++ src/os/exec/exec.go | 10 +++++++ src/os/exec/lp_plan9.go | 4 +++ src/os/exec/lp_unix.go | 4 +++ src/os/exec/lp_windows.go | 8 ++++++ 5 files changed, 82 insertions(+) diff --git a/src/os/exec/dot_test.go b/src/os/exec/dot_test.go index ed4bad23b1cca2..86e9cbb4cfdf4f 100644 --- a/src/os/exec/dot_test.go +++ b/src/os/exec/dot_test.go @@ -178,4 +178,60 @@ func TestLookPath(t *testing.T) { } } }) + + checker := func(test string) func(t *testing.T) { + return func(t *testing.T) { + t.Helper() + t.Logf("PATH=%s", os.Getenv("PATH")) + p, err := LookPath(test) + if err == nil { + t.Errorf("%q: error expected, got nil", test) + } + if p != "" { + t.Errorf("%q: path returned should be \"\". Got %q", test, p) + } + } + } + + // Reference behavior for the next test + t.Run(pathVar+"=$OTHER2", func(t *testing.T) { + t.Run("empty", checker("")) + t.Run("dot", checker(".")) + t.Run("dotdot1", checker("abc/..")) + t.Run("dotdot2", checker("..")) + }) + + // Test the behavior when PATH contains an executable file which is not a directory + t.Run(pathVar+"=exe", func(t *testing.T) { + // Inject an executable file (not a directory) in PATH. + // Use our own binary os.Args[0]. + testenv.MustHaveExec(t) + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + + t.Setenv(pathVar, exe) + t.Run("empty", checker("")) + t.Run("dot", checker(".")) + t.Run("dotdot1", checker("abc/..")) + t.Run("dotdot2", checker("..")) + }) + + // Test the behavior when PATH contains an executable file which is not a directory + t.Run(pathVar+"=exe/xx", func(t *testing.T) { + // Inject an executable file (not a directory) in PATH. + // Use our own binary os.Args[0]. + testenv.MustHaveExec(t) + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + + t.Setenv(pathVar, filepath.Join(exe, "xx")) + t.Run("empty", checker("")) + t.Run("dot", checker(".")) + t.Run("dotdot1", checker("abc/..")) + t.Run("dotdot2", checker("..")) + }) } diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index da9f68fe28f14b..a5e2dec24b22c2 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -1310,3 +1310,13 @@ func addCriticalEnv(env []string) []string { // Code should use errors.Is(err, ErrDot), not err == ErrDot, // to test whether a returned error err is due to this condition. var ErrDot = errors.New("cannot run executable found relative to current directory") + +// validateLookPath excludes paths that can't be valid +// executable names. See issue #74466 and CVE-2025-47906. +func validateLookPath(s string) error { + switch s { + case "", ".", "..": + return ErrNotFound + } + return nil +} diff --git a/src/os/exec/lp_plan9.go b/src/os/exec/lp_plan9.go index 87359b3551d32f..0430af9eefeb42 100644 --- a/src/os/exec/lp_plan9.go +++ b/src/os/exec/lp_plan9.go @@ -36,6 +36,10 @@ func findExecutable(file string) error { // As of Go 1.19, LookPath will instead return that path along with an error satisfying // [errors.Is](err, [ErrDot]). See the package documentation for more details. func LookPath(file string) (string, error) { + if err := validateLookPath(file); err != nil { + return "", &Error{file, err} + } + // skip the path lookup for these prefixes skip := []string{"/", "#", "./", "../"} diff --git a/src/os/exec/lp_unix.go b/src/os/exec/lp_unix.go index 8617d45e983e6e..e5fddbafe21b94 100644 --- a/src/os/exec/lp_unix.go +++ b/src/os/exec/lp_unix.go @@ -54,6 +54,10 @@ func LookPath(file string) (string, error) { // (only bypass the path if file begins with / or ./ or ../) // but that would not match all the Unix shells. + if err := validateLookPath(file); err != nil { + return "", &Error{file, err} + } + if strings.Contains(file, "/") { err := findExecutable(file) if err == nil { diff --git a/src/os/exec/lp_windows.go b/src/os/exec/lp_windows.go index 0e058d41b0c11f..4b4e297112481b 100644 --- a/src/os/exec/lp_windows.go +++ b/src/os/exec/lp_windows.go @@ -68,6 +68,10 @@ func findExecutable(file string, exts []string) (string, error) { // As of Go 1.19, LookPath will instead return that path along with an error satisfying // [errors.Is](err, [ErrDot]). See the package documentation for more details. func LookPath(file string) (string, error) { + if err := validateLookPath(file); err != nil { + return "", &Error{file, err} + } + return lookPath(file, pathExt()) } @@ -81,6 +85,10 @@ func LookPath(file string) (string, error) { // "C:\foo\example.com" would be returned as-is even if the // program is actually "C:\foo\example.com.exe". func lookExtensions(path, dir string) (string, error) { + if err := validateLookPath(path); err != nil { + return "", &Error{path, err} + } + if filepath.Base(path) == path { path = "." + string(filepath.Separator) + path } From 8a924caaf348fdc366bab906424616b2974ad4e9 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 23 Jul 2025 14:26:54 -0700 Subject: [PATCH 6/7] [release-branch.go1.23] database/sql: avoid closing Rows while scan is in progress A database/sql/driver.Rows can return database-owned data from Rows.Next. The driver.Rows documentation doesn't explicitly document the lifetime guarantees for this data, but a reasonable expectation is that the caller of Next should only access it until the next call to Rows.Close or Rows.Next. Avoid violating that constraint when a query is cancelled while a call to database/sql.Rows.Scan (note the difference between the two different Rows types!) is in progress. We previously took care to avoid closing a driver.Rows while the user has access to driver-owned memory via a RawData, but we could still close a driver.Rows while a Scan call was in the process of reading previously-returned driver-owned data. Update the fake DB used in database/sql tests to invalidate returned data to help catch other places we might be incorrectly retaining it. Updates #74831 Fixes #74832 Change-Id: Ice45b5fad51b679c38e3e1d21ef39156b56d6037 Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/2540 Reviewed-by: Roland Shoemaker Reviewed-by: Neal Patel Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/2601 Reviewed-on: https://go-review.googlesource.com/c/go/+/693558 TryBot-Bypass: Dmitri Shuralyov Reviewed-by: Mark Freeman Reviewed-by: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov --- src/database/sql/convert.go | 2 -- src/database/sql/fakedb_test.go | 47 ++++++++++++------------ src/database/sql/sql.go | 26 +++++++------- src/database/sql/sql_test.go | 64 ++++++++++++++++++++++++++++----- 4 files changed, 90 insertions(+), 49 deletions(-) diff --git a/src/database/sql/convert.go b/src/database/sql/convert.go index c261046b187e52..396833c2fc8009 100644 --- a/src/database/sql/convert.go +++ b/src/database/sql/convert.go @@ -335,7 +335,6 @@ func convertAssignRows(dest, src any, rows *Rows) error { if rows == nil { return errors.New("invalid context to convert cursor rows, missing parent *Rows") } - rows.closemu.Lock() *d = Rows{ dc: rows.dc, releaseConn: func(error) {}, @@ -351,7 +350,6 @@ func convertAssignRows(dest, src any, rows *Rows) error { parentCancel() } } - rows.closemu.Unlock() return nil } } diff --git a/src/database/sql/fakedb_test.go b/src/database/sql/fakedb_test.go index 3dfcd447b52bca..003e6c62986f31 100644 --- a/src/database/sql/fakedb_test.go +++ b/src/database/sql/fakedb_test.go @@ -5,6 +5,7 @@ package sql import ( + "bytes" "context" "database/sql/driver" "errors" @@ -15,7 +16,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "testing" "time" ) @@ -91,8 +91,6 @@ func (cc *fakeDriverCtx) OpenConnector(name string) (driver.Connector, error) { type fakeDB struct { name string - useRawBytes atomic.Bool - mu sync.Mutex tables map[string]*table badConn bool @@ -684,8 +682,6 @@ func (c *fakeConn) PrepareContext(ctx context.Context, query string) (driver.Stm switch cmd { case "WIPE": // Nothing - case "USE_RAWBYTES": - c.db.useRawBytes.Store(true) case "SELECT": stmt, err = c.prepareSelect(stmt, parts) case "CREATE": @@ -789,9 +785,6 @@ func (s *fakeStmt) ExecContext(ctx context.Context, args []driver.NamedValue) (d case "WIPE": db.wipe() return driver.ResultNoRows, nil - case "USE_RAWBYTES": - s.c.db.useRawBytes.Store(true) - return driver.ResultNoRows, nil case "CREATE": if err := db.createTable(s.table, s.colName, s.colType); err != nil { return nil, err @@ -1076,10 +1069,9 @@ type rowsCursor struct { errPos int err error - // a clone of slices to give out to clients, indexed by the - // original slice's first byte address. we clone them - // just so we're able to corrupt them on close. - bytesClone map[*byte][]byte + // Data returned to clients. + // We clone and stash it here so it can be invalidated by Close and Next. + driverOwnedMemory [][]byte // Every operation writes to line to enable the race detector // check for data races. @@ -1096,9 +1088,19 @@ func (rc *rowsCursor) touchMem() { rc.line++ } +func (rc *rowsCursor) invalidateDriverOwnedMemory() { + for _, buf := range rc.driverOwnedMemory { + for i := range buf { + buf[i] = 'x' + } + } + rc.driverOwnedMemory = nil +} + func (rc *rowsCursor) Close() error { rc.touchMem() rc.parentMem.touchMem() + rc.invalidateDriverOwnedMemory() rc.closed = true return rc.closeErr } @@ -1129,6 +1131,8 @@ func (rc *rowsCursor) Next(dest []driver.Value) error { if rc.posRow >= len(rc.rows[rc.posSet]) { return io.EOF // per interface spec } + // Corrupt any previously returned bytes. + rc.invalidateDriverOwnedMemory() for i, v := range rc.rows[rc.posSet][rc.posRow].cols { // TODO(bradfitz): convert to subset types? naah, I // think the subset types should only be input to @@ -1136,20 +1140,13 @@ func (rc *rowsCursor) Next(dest []driver.Value) error { // a wider range of types coming out of drivers. all // for ease of drivers, and to prevent drivers from // messing up conversions or doing them differently. - dest[i] = v - - if bs, ok := v.([]byte); ok && !rc.db.useRawBytes.Load() { - if rc.bytesClone == nil { - rc.bytesClone = make(map[*byte][]byte) - } - clone, ok := rc.bytesClone[&bs[0]] - if !ok { - clone = make([]byte, len(bs)) - copy(clone, bs) - rc.bytesClone[&bs[0]] = clone - } - dest[i] = clone + if bs, ok := v.([]byte); ok { + // Clone []bytes and stash for later invalidation. + bs = bytes.Clone(bs) + rc.driverOwnedMemory = append(rc.driverOwnedMemory, bs) + v = bs } + dest[i] = v } return nil } diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index c247a9b506bfab..3346ad48f1b33e 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -3360,38 +3360,36 @@ func (rs *Rows) Scan(dest ...any) error { // without calling Next. return fmt.Errorf("sql: Scan called without calling Next (closemuScanHold)") } + rs.closemu.RLock() + rs.raw = rs.raw[:0] + err := rs.scanLocked(dest...) + if err == nil && scanArgsContainRawBytes(dest) { + rs.closemuScanHold = true + } else { + rs.closemu.RUnlock() + } + return err +} +func (rs *Rows) scanLocked(dest ...any) error { if rs.lasterr != nil && rs.lasterr != io.EOF { - rs.closemu.RUnlock() return rs.lasterr } if rs.closed { - err := rs.lasterrOrErrLocked(errRowsClosed) - rs.closemu.RUnlock() - return err - } - - if scanArgsContainRawBytes(dest) { - rs.closemuScanHold = true - rs.raw = rs.raw[:0] - } else { - rs.closemu.RUnlock() + return rs.lasterrOrErrLocked(errRowsClosed) } if rs.lastcols == nil { - rs.closemuRUnlockIfHeldByScan() return errors.New("sql: Scan called without calling Next") } if len(dest) != len(rs.lastcols) { - rs.closemuRUnlockIfHeldByScan() return fmt.Errorf("sql: expected %d destination arguments in Scan, not %d", len(rs.lastcols), len(dest)) } for i, sv := range rs.lastcols { err := convertAssignRows(dest[i], sv, rs) if err != nil { - rs.closemuRUnlockIfHeldByScan() return fmt.Errorf(`sql: Scan error on column index %d, name %q: %w`, i, rs.rowsi.Columns()[i], err) } } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 110a2bae5bd247..236bbbb1746d51 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -5,6 +5,7 @@ package sql import ( + "bytes" "context" "database/sql/driver" "errors" @@ -4446,10 +4447,6 @@ func testContextCancelDuringRawBytesScan(t *testing.T, mode string) { db := newTestDB(t, "people") defer closeDB(t, db) - if _, err := db.Exec("USE_RAWBYTES"); err != nil { - t.Fatal(err) - } - // cancel used to call close asynchronously. // This test checks that it waits so as not to interfere with RawBytes. ctx, cancel := context.WithCancel(context.Background()) @@ -4541,6 +4538,61 @@ func TestContextCancelBetweenNextAndErr(t *testing.T) { } } +type testScanner struct { + scanf func(src any) error +} + +func (ts testScanner) Scan(src any) error { return ts.scanf(src) } + +func TestContextCancelDuringScan(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + scanStart := make(chan any) + scanEnd := make(chan error) + scanner := &testScanner{ + scanf: func(src any) error { + scanStart <- src + return <-scanEnd + }, + } + + // Start a query, and pause it mid-scan. + want := []byte("Alice") + r, err := db.QueryContext(ctx, "SELECT|people|name|name=?", string(want)) + if err != nil { + t.Fatal(err) + } + if !r.Next() { + t.Fatalf("r.Next() = false, want true") + } + go func() { + r.Scan(scanner) + }() + got := <-scanStart + defer close(scanEnd) + gotBytes, ok := got.([]byte) + if !ok { + t.Fatalf("r.Scan returned %T, want []byte", got) + } + if !bytes.Equal(gotBytes, want) { + t.Fatalf("before cancel: r.Scan returned %q, want %q", gotBytes, want) + } + + // Cancel the query. + // Sleep to give it a chance to finish canceling. + cancel() + time.Sleep(10 * time.Millisecond) + + // Cancelling the query should not have changed the result. + if !bytes.Equal(gotBytes, want) { + t.Fatalf("after cancel: r.Scan result is now %q, want %q", gotBytes, want) + } +} + func TestNilErrorAfterClose(t *testing.T) { db := newTestDB(t, "people") defer closeDB(t, db) @@ -4574,10 +4626,6 @@ func TestRawBytesReuse(t *testing.T) { db := newTestDB(t, "people") defer closeDB(t, db) - if _, err := db.Exec("USE_RAWBYTES"); err != nil { - t.Fatal(err) - } - var raw RawBytes // The RawBytes in this query aliases driver-owned memory. From dd8b7ad9268c2fbde675132a41b4e4da02eef94d Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Wed, 6 Aug 2025 11:05:37 -0700 Subject: [PATCH 7/7] [release-branch.go1.23] go1.23.12 Change-Id: Ibd36536a8c9d9c8d90f466d420a30d4e3162ff25 Reviewed-on: https://go-review.googlesource.com/c/go/+/693698 TryBot-Bypass: Gopher Robot Auto-Submit: Gopher Robot Reviewed-by: Mark Freeman Reviewed-by: Dmitri Shuralyov --- VERSION | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index e6644419e6a038..e1a19c557bc5c3 100644 --- a/VERSION +++ b/VERSION @@ -1,2 +1,2 @@ -go1.23.11 -time 2025-07-02T21:47:15Z +go1.23.12 +time 2025-08-01T19:18:26Z