diff --git a/VERSION b/VERSION index 2c03dea4..48fae286 100644 --- a/VERSION +++ b/VERSION @@ -1,2 +1,2 @@ -go1.24.5 -time 2025-07-02T21:47:15Z +go1.24.6 +time 2025-08-01T19:18:26Z diff --git a/src/cmd/internal/obj/arm64/obj7.go b/src/cmd/internal/obj/arm64/obj7.go index 368a631f..ea1a35b0 100644 --- a/src/cmd/internal/obj/arm64/obj7.go +++ b/src/cmd/internal/obj/arm64/obj7.go @@ -907,18 +907,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', diff --git a/src/database/sql/convert.go b/src/database/sql/convert.go index 65fdfe6f..26b139ab 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 3dfcd447..003e6c62 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 eddb647e..1cbb52ce 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -3368,38 +3368,36 @@ func (rs *Rows) Scan(dest ...any) error { // without calling Next. return fmt.Errorf("sql: Scan called without calling Next (closemuScanHold)") } + rs.closemu.RLock() - - 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.raw = rs.raw[:0] + err := rs.scanLocked(dest...) + if err == nil && scanArgsContainRawBytes(dest) { rs.closemuScanHold = true - rs.raw = rs.raw[:0] } else { rs.closemu.RUnlock() } + return err +} + +func (rs *Rows) scanLocked(dest ...any) error { + if rs.lasterr != nil && rs.lasterr != io.EOF { + return rs.lasterr + } + if rs.closed { + 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 db1d8b3c..cf3dcc74 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. diff --git a/src/os/exec/dot_test.go b/src/os/exec/dot_test.go index 1bf0d9c7..b95639e6 100644 --- a/src/os/exec/dot_test.go +++ b/src/os/exec/dot_test.go @@ -177,4 +177,48 @@ 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]. + t.Setenv(pathVar, testenv.Executable(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/xx", func(t *testing.T) { + // Inject an executable file (not a directory) in PATH. + // Use our own binary os.Args[0]. + t.Setenv(pathVar, filepath.Join(testenv.Executable(t), "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 fecfc97d..3decdc75 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -1328,3 +1328,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 87359b35..0430af9e 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 8617d45e..e5fddbaf 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 12256743..e01e7bbb 100644 --- a/src/os/exec/lp_windows.go +++ b/src/os/exec/lp_windows.go @@ -67,6 +67,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()) } @@ -80,6 +84,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 } diff --git a/src/os/user/user_windows_test.go b/src/os/user/user_windows_test.go index c7150337..c8ee10cb 100644 --- a/src/os/user/user_windows_test.go +++ b/src/os/user/user_windows_test.go @@ -7,6 +7,7 @@ package user import ( "crypto/rand" "encoding/base64" + "encoding/binary" "errors" "fmt" "internal/syscall/windows" @@ -15,11 +16,92 @@ import ( "os/exec" "runtime" "strconv" + "strings" "syscall" "testing" + "unicode" + "unicode/utf8" "unsafe" ) +// addUserAccount creates a local user account. +// It returns the name and password of the new account. +// Multiple programs or goroutines calling addUserAccount simultaneously will not choose the same directory. +func addUserAccount(t *testing.T) (name, password string) { + t.TempDir() + pattern := t.Name() + // Windows limits the user name to 20 characters, + // leave space for a 4 digits random suffix. + const maxNameLen, suffixLen = 20, 4 + pattern = pattern[:min(len(pattern), maxNameLen-suffixLen)] + // Drop unusual characters from the account name. + mapper := func(r rune) rune { + if r < utf8.RuneSelf { + if '0' <= r && r <= '9' || + 'a' <= r && r <= 'z' || + 'A' <= r && r <= 'Z' { + return r + } + } else if unicode.IsLetter(r) || unicode.IsNumber(r) { + return r + } + return -1 + } + pattern = strings.Map(mapper, pattern) + + // Generate a long random password. + var pwd [33]byte + rand.Read(pwd[:]) + // Add special chars to ensure it satisfies password requirements. + password = base64.StdEncoding.EncodeToString(pwd[:]) + "_-As@!%*(1)4#2" + password16, err := syscall.UTF16PtrFromString(password) + if err != nil { + t.Fatal(err) + } + + try := 0 + for { + // Calculate a random suffix to append to the user name. + var suffix [2]byte + rand.Read(suffix[:]) + suffixStr := strconv.FormatUint(uint64(binary.LittleEndian.Uint16(suffix[:])), 10) + name := pattern + suffixStr[:min(len(suffixStr), suffixLen)] + name16, err := syscall.UTF16PtrFromString(name) + if err != nil { + t.Fatal(err) + } + // Create user. + userInfo := windows.UserInfo1{ + Name: name16, + Password: password16, + Priv: windows.USER_PRIV_USER, + } + err = windows.NetUserAdd(nil, 1, (*byte)(unsafe.Pointer(&userInfo)), nil) + if errors.Is(err, syscall.ERROR_ACCESS_DENIED) { + t.Skip("skipping test; don't have permission to create user") + } + // If the user already exists, try again with a different name. + if errors.Is(err, windows.NERR_UserExists) { + if try++; try < 1000 { + t.Log("user already exists, trying again with a different name") + continue + } + } + if err != nil { + t.Fatalf("NetUserAdd failed: %v", err) + } + // Delete the user when the test is done. + t.Cleanup(func() { + if err := windows.NetUserDel(nil, name16); err != nil { + if !errors.Is(err, windows.NERR_UserNotFound) { + t.Fatal(err) + } + } + }) + return name, password + } +} + // windowsTestAccount creates a test user and returns a token for that user. // If the user already exists, it will be deleted and recreated. // The caller is responsible for closing the token. @@ -31,47 +113,15 @@ func windowsTestAccount(t *testing.T) (syscall.Token, *User) { // See https://dev.go/issue/70396. t.Skip("skipping non-hermetic test outside of Go builders") } - const testUserName = "GoStdTestUser01" - var password [33]byte - rand.Read(password[:]) - // Add special chars to ensure it satisfies password requirements. - pwd := base64.StdEncoding.EncodeToString(password[:]) + "_-As@!%*(1)4#2" - name, err := syscall.UTF16PtrFromString(testUserName) + name, password := addUserAccount(t) + name16, err := syscall.UTF16PtrFromString(name) if err != nil { t.Fatal(err) } - pwd16, err := syscall.UTF16PtrFromString(pwd) + pwd16, err := syscall.UTF16PtrFromString(password) if err != nil { t.Fatal(err) } - userInfo := windows.UserInfo1{ - Name: name, - Password: pwd16, - Priv: windows.USER_PRIV_USER, - } - // Create user. - err = windows.NetUserAdd(nil, 1, (*byte)(unsafe.Pointer(&userInfo)), nil) - if errors.Is(err, syscall.ERROR_ACCESS_DENIED) { - t.Skip("skipping test; don't have permission to create user") - } - if errors.Is(err, windows.NERR_UserExists) { - // User already exists, delete and recreate. - if err = windows.NetUserDel(nil, name); err != nil { - t.Fatal(err) - } - if err = windows.NetUserAdd(nil, 1, (*byte)(unsafe.Pointer(&userInfo)), nil); err != nil { - t.Fatal(err) - } - } else if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { - if err = windows.NetUserDel(nil, name); err != nil { - if !errors.Is(err, windows.NERR_UserNotFound) { - t.Fatal(err) - } - } - }) domain, err := syscall.UTF16PtrFromString(".") if err != nil { t.Fatal(err) @@ -79,13 +129,13 @@ func windowsTestAccount(t *testing.T) (syscall.Token, *User) { const LOGON32_PROVIDER_DEFAULT = 0 const LOGON32_LOGON_INTERACTIVE = 2 var token syscall.Token - if err = windows.LogonUser(name, domain, pwd16, LOGON32_LOGON_INTERACTIVE, LOGON32_PROVIDER_DEFAULT, &token); err != nil { + if err = windows.LogonUser(name16, domain, pwd16, LOGON32_LOGON_INTERACTIVE, LOGON32_PROVIDER_DEFAULT, &token); err != nil { t.Fatal(err) } t.Cleanup(func() { token.Close() }) - usr, err := Lookup(testUserName) + usr, err := Lookup(name) if err != nil { t.Fatal(err) } diff --git a/src/runtime/malloc.go b/src/runtime/malloc.go index 73d663f7..ba9e02e9 100644 --- a/src/runtime/malloc.go +++ b/src/runtime/malloc.go @@ -1544,12 +1544,13 @@ func mallocgcLarge(size uintptr, typ *_type, needzero bool) (unsafe.Pointer, uin size = span.elemsize x := unsafe.Pointer(span.base()) - // Ensure that the stores above that initialize x to - // type-safe memory and set the heap bits occur before - // the caller can make x observable to the garbage - // collector. Otherwise, on weakly ordered machines, - // the garbage collector could follow a pointer to x, - // but see uninitialized memory or stale heap bits. + // Ensure that the store above that sets largeType to + // nil happens before the caller can make x observable + // to the garbage collector. + // + // Otherwise, on weakly ordered machines, the garbage + // collector could follow a pointer to x, but see a stale + // largeType value. publicationBarrier() // As x and the heap bits are initialized, update // freeIndexForScan now so x is seen by the GC @@ -1592,22 +1593,33 @@ func mallocgcLarge(size uintptr, typ *_type, needzero bool) (unsafe.Pointer, uin } // Objects can be zeroed late in a context where preemption can occur. - // If the object contains pointers, its pointer data must be cleared - // or otherwise indicate that the GC shouldn't scan it. + // // x will keep the memory alive. - if noscan := typ == nil || !typ.Pointers(); !noscan || (needzero && span.needzero != 0) { + if needzero && span.needzero != 0 { // N.B. size == fullSize always in this case. memclrNoHeapPointersChunked(size, x) // This is a possible preemption point: see #47302 - - // Finish storing the type information for this case. - mp := acquirem() - if !noscan { - getMCache(mp).scanAlloc += heapSetTypeLarge(uintptr(x), size, typ, span) - } - // Publish the object with the now-zeroed memory. - publicationBarrier() - releasem(mp) } + + // Set the type and run the publication barrier while non-preemptible. We need to make + // sure that between heapSetTypeLarge and publicationBarrier we cannot get preempted, + // otherwise the GC could potentially observe non-zeroed memory but largeType set on weak + // memory architectures. + // + // The GC can also potentially observe non-zeroed memory if conservative scanning spuriously + // observes a partially-allocated object, see the freeIndexForScan update above. This case is + // handled by synchronization inside heapSetTypeLarge. + mp = acquirem() + if typ != nil && typ.Pointers() { + // Finish storing the type information, now that we're certain the memory is zeroed. + getMCache(mp).scanAlloc += heapSetTypeLarge(uintptr(x), size, typ, span) + } + // Publish the object again, now with zeroed memory and initialized type information. + // + // Even if we didn't update any type information, this is necessary to ensure that, for example, + // x written to a global without any synchronization still results in other goroutines observing + // zeroed memory. + publicationBarrier() + releasem(mp) return x, size } diff --git a/src/runtime/mbitmap.go b/src/runtime/mbitmap.go index 148b2d78..4bffabec 100644 --- a/src/runtime/mbitmap.go +++ b/src/runtime/mbitmap.go @@ -191,7 +191,9 @@ func (span *mspan) typePointersOfUnchecked(addr uintptr) typePointers { typ = *(**_type)(unsafe.Pointer(addr)) addr += mallocHeaderSize } else { - typ = span.largeType + // Synchronize with allocator, in case this came from the conservative scanner. + // See heapSetTypeLarge for more details. + typ = (*_type)(atomic.Loadp(unsafe.Pointer(&span.largeType))) if typ == nil { // Allow a nil type here for delayed zeroing. See mallocgc. return typePointers{} @@ -739,8 +741,56 @@ func heapSetTypeSmallHeader(x, dataSize uintptr, typ *_type, header **_type, spa func heapSetTypeLarge(x, dataSize uintptr, typ *_type, span *mspan) uintptr { gctyp := typ - // Write out the header. - span.largeType = gctyp + // Write out the header atomically to synchronize with the garbage collector. + // + // This atomic store is paired with an atomic load in typePointersOfUnchecked. + // This store ensures that initializing x's memory cannot be reordered after + // this store. Meanwhile the load in typePointersOfUnchecked ensures that + // reading x's memory cannot be reordered before largeType is loaded. Together, + // these two operations guarantee that the garbage collector can only see + // initialized memory if largeType is non-nil. + // + // Gory details below... + // + // Ignoring conservative scanning for a moment, this store need not be atomic + // if we have a publication barrier on our side. This is because the garbage + // collector cannot observe x unless: + // 1. It stops this goroutine and scans its stack, or + // 2. We return from mallocgc and publish the pointer somewhere. + // Either case requires a write on our side, followed by some synchronization + // followed by a read by the garbage collector. + // + // In case (1), the garbage collector can only observe a nil largeType, since it + // had to stop our goroutine when it was preemptible during zeroing. For the + // duration of the zeroing, largeType is nil and the object has nothing interesting + // for the garbage collector to look at, so the garbage collector will not access + // the object at all. + // + // In case (2), the garbage collector can also observe a nil largeType. This + // might happen if the object was newly allocated, and a new GC cycle didn't start + // (that would require a global barrier, STW). In this case, the garbage collector + // will once again ignore the object, and that's safe because objects are + // allocate-black. + // + // However, the garbage collector can also observe a non-nil largeType in case (2). + // This is still okay, since to access the object's memory, it must have first + // loaded the object's pointer from somewhere. This makes the access of the object's + // memory a data-dependent load, and our publication barrier in the allocator + // guarantees that a data-dependent load must observe a version of the object's + // data from after the publication barrier executed. + // + // Unfortunately conservative scanning is a problem. There's no guarantee of a + // data dependency as in case (2) because conservative scanning can produce pointers + // 'out of thin air' in that it need not have been written somewhere by the allocating + // thread first. It might not even be a pointer, or it could be a pointer written to + // some stack location long ago. This is the fundamental reason why we need + // explicit synchronization somewhere in this whole mess. We choose to put that + // synchronization on largeType. + // + // As described at the very top, the treating largeType as an atomic variable, on + // both the reader and writer side, is sufficient to ensure that only initialized + // memory at x will be observed if largeType is non-nil. + atomic.StorepNoWB(unsafe.Pointer(&span.largeType), unsafe.Pointer(gctyp)) if doubleCheckHeapSetType { doubleCheckHeapType(x, dataSize, typ, &span.largeType, span) } diff --git a/src/runtime/proc.go b/src/runtime/proc.go index 42200dcb..5016c71f 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -1018,6 +1018,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 } @@ -3297,6 +3319,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() @@ -3465,7 +3492,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 @@ -3597,6 +3628,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) @@ -4037,6 +4071,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 9c7065ee..5ac868fb 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -562,6 +562,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