From 828be9a74319a08e9098b8a0e60119435bd87adb Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Thu, 20 Oct 2022 10:22:10 -0400 Subject: [release-branch.go1.18] cmd/go/internal/modload: update TestQueryImport to pass with tagged versions of x/net For #48523. Change-Id: Ied35d15462cbae1002e1db1e6e119a6c9f8323da Reviewed-on: https://go-review.googlesource.com/c/go/+/444156 Run-TryBot: Bryan Mills Reviewed-by: Than McIntosh TryBot-Result: Gopher Robot Auto-Submit: Bryan Mills (cherry picked from commit 3e6ca3a506fc89f19277b3c19b751847b3864185) Reviewed-on: https://go-review.googlesource.com/c/go/+/444436 Reviewed-by: Heschi Kreinick --- src/cmd/go/internal/modload/import_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmd/go/internal/modload/import_test.go b/src/cmd/go/internal/modload/import_test.go index 65a889ec52..eb4f5d64d3 100644 --- a/src/cmd/go/internal/modload/import_test.go +++ b/src/cmd/go/internal/modload/import_test.go @@ -27,7 +27,7 @@ var importTests = []struct { }, { path: "golang.org/x/net", - err: `module golang.org/x/net@.* found \(v0.0.0-.*\), but does not contain package golang.org/x/net`, + err: `module golang.org/x/net@.* found \(v[01]\.\d+\.\d+\), but does not contain package golang.org/x/net`, }, { path: "golang.org/x/text", -- cgit v1.2.1 From 2c2952aea8fb041a78689c4c5f917fd166f8310b Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Tue, 18 Oct 2022 12:01:18 -0400 Subject: [release-branch.go1.18] runtime: always keep global reference to mp until mexit completes Ms are allocated via standard heap allocation (`new(m)`), which means we must keep them alive (i.e., reachable by the GC) until we are completely done using them. Ms are primarily reachable through runtime.allm. However, runtime.mexit drops the M from allm fairly early, long before it is done using the M structure. If that was the last reference to the M, it is now at risk of being freed by the GC and used for some other allocation, leading to memory corruption. Ms with a Go-allocated stack coincidentally already keep a reference to the M in sched.freem, so that the stack can be freed lazily. This reference has the side effect of keeping this Ms reachable. However, Ms with an OS stack skip this and are at risk of corruption. Fix this lifetime by extending sched.freem use to all Ms, with the value of mp.freeWait determining whether the stack needs to be freed or not. For #56243. Fixes #56308. Change-Id: Ic0c01684775f5646970df507111c9abaac0ba52e Reviewed-on: https://go-review.googlesource.com/c/go/+/443716 TryBot-Result: Gopher Robot Run-TryBot: Michael Pratt Reviewed-by: Michael Knyszek (cherry picked from commit e252dcf9d38ce9192bccacb7e33867cbfbd22b6c) Reviewed-on: https://go-review.googlesource.com/c/go/+/443816 Reviewed-by: Austin Clements --- src/runtime/os3_solaris.go | 3 ++- src/runtime/os_aix.go | 3 ++- src/runtime/os_js.go | 3 ++- src/runtime/os_openbsd_syscall2.go | 5 ++-- src/runtime/os_plan9.go | 2 +- src/runtime/os_windows.go | 2 +- src/runtime/proc.go | 48 ++++++++++++++++++++++---------------- src/runtime/runtime2.go | 9 ++++++- src/runtime/stubs2.go | 9 ++++--- src/runtime/sys_darwin.go | 3 ++- src/runtime/sys_dragonfly_amd64.s | 2 +- src/runtime/sys_freebsd_386.s | 2 +- src/runtime/sys_freebsd_amd64.s | 2 +- src/runtime/sys_freebsd_arm.s | 2 +- src/runtime/sys_freebsd_arm64.s | 2 +- src/runtime/sys_linux_386.s | 2 +- src/runtime/sys_linux_amd64.s | 2 +- src/runtime/sys_linux_arm.s | 2 +- src/runtime/sys_linux_arm64.s | 2 +- src/runtime/sys_linux_mips64x.s | 2 +- src/runtime/sys_linux_mipsx.s | 2 +- src/runtime/sys_linux_ppc64x.s | 2 +- src/runtime/sys_linux_riscv64.s | 2 +- src/runtime/sys_linux_s390x.s | 2 +- src/runtime/sys_netbsd_386.s | 2 +- src/runtime/sys_netbsd_amd64.s | 2 +- src/runtime/sys_netbsd_arm.s | 2 +- src/runtime/sys_netbsd_arm64.s | 2 +- src/runtime/sys_openbsd2.go | 3 ++- src/runtime/sys_openbsd_mips64.s | 2 +- 30 files changed, 76 insertions(+), 52 deletions(-) diff --git a/src/runtime/os3_solaris.go b/src/runtime/os3_solaris.go index 4aba0ff64b..3ccce776ca 100644 --- a/src/runtime/os3_solaris.go +++ b/src/runtime/os3_solaris.go @@ -7,6 +7,7 @@ package runtime import ( "internal/abi" "internal/goarch" + "runtime/internal/atomic" "unsafe" ) @@ -184,7 +185,7 @@ func newosproc(mp *m) { } } -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { // We should never reach exitThread on Solaris because we let // libc clean up threads. throw("exitThread") diff --git a/src/runtime/os_aix.go b/src/runtime/os_aix.go index 292ff94795..41352b3a5a 100644 --- a/src/runtime/os_aix.go +++ b/src/runtime/os_aix.go @@ -8,6 +8,7 @@ package runtime import ( "internal/abi" + "runtime/internal/atomic" "unsafe" ) @@ -232,7 +233,7 @@ func newosproc(mp *m) { } -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { // We should never reach exitThread on AIX because we let // libc clean up threads. throw("exitThread") diff --git a/src/runtime/os_js.go b/src/runtime/os_js.go index 9ed916705b..6f358d36d6 100644 --- a/src/runtime/os_js.go +++ b/src/runtime/os_js.go @@ -7,6 +7,7 @@ package runtime import ( + "runtime/internal/atomic" "unsafe" ) @@ -35,7 +36,7 @@ func usleep_no_g(usec uint32) { usleep(usec) } -func exitThread(wait *uint32) +func exitThread(wait *atomic.Uint32) type mOS struct{} diff --git a/src/runtime/os_openbsd_syscall2.go b/src/runtime/os_openbsd_syscall2.go index 99542fb2de..810d599508 100644 --- a/src/runtime/os_openbsd_syscall2.go +++ b/src/runtime/os_openbsd_syscall2.go @@ -7,6 +7,7 @@ package runtime import ( + "runtime/internal/atomic" "unsafe" ) @@ -48,11 +49,11 @@ func open(name *byte, mode, perm int32) int32 // return value is only set on linux to be used in osinit() func madvise(addr unsafe.Pointer, n uintptr, flags int32) int32 -// exitThread terminates the current thread, writing *wait = 0 when +// exitThread terminates the current thread, writing *wait = freeMStack when // the stack is safe to reclaim. // //go:noescape -func exitThread(wait *uint32) +func exitThread(wait *atomic.Uint32) //go:noescape func obsdsigprocmask(how int32, new sigset) sigset diff --git a/src/runtime/os_plan9.go b/src/runtime/os_plan9.go index 975d460a7d..2bb6ec0fce 100644 --- a/src/runtime/os_plan9.go +++ b/src/runtime/os_plan9.go @@ -458,7 +458,7 @@ func newosproc(mp *m) { } } -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { // We should never reach exitThread on Plan 9 because we let // the OS clean up threads. throw("exitThread") diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go index c76add7802..b49e9baec5 100644 --- a/src/runtime/os_windows.go +++ b/src/runtime/os_windows.go @@ -939,7 +939,7 @@ func newosproc0(mp *m, stk unsafe.Pointer) { throw("bad newosproc0") } -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { // We should never reach exitThread on Windows because we let // the OS clean up threads. throw("exitThread") diff --git a/src/runtime/proc.go b/src/runtime/proc.go index b997a467ba..cae15bc8e2 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -1508,19 +1508,18 @@ func mexit(osStack bool) { } throw("m not found in allm") found: - if !osStack { - // Delay reaping m until it's done with the stack. - // - // If this is using an OS stack, the OS will free it - // so there's no need for reaping. - atomic.Store(&m.freeWait, 1) - // Put m on the free list, though it will not be reaped until - // freeWait is 0. Note that the free list must not be linked - // through alllink because some functions walk allm without - // locking, so may be using alllink. - m.freelink = sched.freem - sched.freem = m - } + // Delay reaping m until it's done with the stack. + // + // Put mp on the free list, though it will not be reaped while freeWait + // is freeMWait. mp is no longer reachable via allm, so even if it is + // on an OS stack, we must keep a reference to mp alive so that the GC + // doesn't free mp while we are still using it. + // + // Note that the free list must not be linked through alllink because + // some functions walk allm without locking, so may be using alllink. + m.freeWait.Store(freeMWait) + m.freelink = sched.freem + sched.freem = m unlock(&sched.lock) atomic.Xadd64(&ncgocall, int64(m.ncgocall)) @@ -1550,6 +1549,9 @@ found: mdestroy(m) if osStack { + // No more uses of mp, so it is safe to drop the reference. + m.freeWait.Store(freeMRef) + // Return from mstart and let the system thread // library free the g0 stack and terminate the thread. return @@ -1721,19 +1723,25 @@ func allocm(_p_ *p, fn func(), id int64) *m { lock(&sched.lock) var newList *m for freem := sched.freem; freem != nil; { - if freem.freeWait != 0 { + wait := freem.freeWait.Load() + if wait == freeMWait { next := freem.freelink freem.freelink = newList newList = freem freem = next continue } - // stackfree must be on the system stack, but allocm is - // reachable off the system stack transitively from - // startm. - systemstack(func() { - stackfree(freem.g0.stack) - }) + // Free the stack if needed. For freeMRef, there is + // nothing to do except drop freem from the sched.freem + // list. + if wait == freeMStack { + // stackfree must be on the system stack, but allocm is + // reachable off the system stack transitively from + // startm. + systemstack(func() { + stackfree(freem.g0.stack) + }) + } freem = freem.freelink } sched.freem = newList diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index b40045e4a5..9cd4777cf8 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -510,6 +510,13 @@ const ( tlsSize = tlsSlots * goarch.PtrSize ) +// Values for m.freeWait. +const ( + freeMStack = 0 // M done, free stack and reference. + freeMRef = 1 // M done, free reference. + freeMWait = 2 // M still in use. +) + type m struct { g0 *g // goroutine with scheduling stack morebuf gobuf // gobuf arg to morestack @@ -540,7 +547,7 @@ type m struct { newSigstack bool // minit on C thread called sigaltstack printlock int8 incgo bool // m is executing a cgo call - freeWait uint32 // if == 0, safe to free g0 and delete m (atomic) + freeWait atomic.Uint32 // Whether it is safe to free g0 and delete m (one of freeMRef, freeMStack, freeMWait) fastrand uint64 needextram bool traceback uint8 diff --git a/src/runtime/stubs2.go b/src/runtime/stubs2.go index 9aa965454d..0d211e2db9 100644 --- a/src/runtime/stubs2.go +++ b/src/runtime/stubs2.go @@ -6,7 +6,10 @@ package runtime -import "unsafe" +import ( + "runtime/internal/atomic" + "unsafe" +) // read calls the read system call. // It returns a non-negative number of bytes written or a negative errno value. @@ -33,8 +36,8 @@ func open(name *byte, mode, perm int32) int32 // return value is only set on linux to be used in osinit() func madvise(addr unsafe.Pointer, n uintptr, flags int32) int32 -// exitThread terminates the current thread, writing *wait = 0 when +// exitThread terminates the current thread, writing *wait = freeMStack when // the stack is safe to reclaim. // //go:noescape -func exitThread(wait *uint32) +func exitThread(wait *atomic.Uint32) diff --git a/src/runtime/sys_darwin.go b/src/runtime/sys_darwin.go index 58b3a9171c..c90cc78968 100644 --- a/src/runtime/sys_darwin.go +++ b/src/runtime/sys_darwin.go @@ -6,6 +6,7 @@ package runtime import ( "internal/abi" + "runtime/internal/atomic" "unsafe" ) @@ -473,7 +474,7 @@ func pthread_cond_signal(c *pthreadcond) int32 { func pthread_cond_signal_trampoline() // Not used on Darwin, but must be defined. -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { } //go:nosplit diff --git a/src/runtime/sys_dragonfly_amd64.s b/src/runtime/sys_dragonfly_amd64.s index d57bc2a7a4..3af0928828 100644 --- a/src/runtime/sys_dragonfly_amd64.s +++ b/src/runtime/sys_dragonfly_amd64.s @@ -65,7 +65,7 @@ TEXT runtime·exit(SB),NOSPLIT,$-8 MOVL $0xf1, 0xf1 // crash RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-8 MOVQ wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_freebsd_386.s b/src/runtime/sys_freebsd_386.s index 97e6d9ab36..d4c4cc7fdb 100644 --- a/src/runtime/sys_freebsd_386.s +++ b/src/runtime/sys_freebsd_386.s @@ -63,7 +63,7 @@ GLOBL exitStack<>(SB),RODATA,$8 DATA exitStack<>+0x00(SB)/4, $0 DATA exitStack<>+0x04(SB)/4, $0 -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVL wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_freebsd_amd64.s b/src/runtime/sys_freebsd_amd64.s index 165e97c60d..57ae0399a5 100644 --- a/src/runtime/sys_freebsd_amd64.s +++ b/src/runtime/sys_freebsd_amd64.s @@ -60,7 +60,7 @@ TEXT runtime·exit(SB),NOSPLIT,$-8 MOVL $0xf1, 0xf1 // crash RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-8 MOVQ wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_freebsd_arm.s b/src/runtime/sys_freebsd_arm.s index b12e47c576..8546eba805 100644 --- a/src/runtime/sys_freebsd_arm.s +++ b/src/runtime/sys_freebsd_arm.s @@ -86,7 +86,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0 MOVW.CS R8, (R8) RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVW wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_freebsd_arm64.s b/src/runtime/sys_freebsd_arm64.s index 1aa09e87ca..7dab3028de 100644 --- a/src/runtime/sys_freebsd_arm64.s +++ b/src/runtime/sys_freebsd_arm64.s @@ -98,7 +98,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 MOVD $0, R0 MOVD R0, (R0) -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOVD wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_linux_386.s b/src/runtime/sys_linux_386.s index 6df812234c..ddc5534352 100644 --- a/src/runtime/sys_linux_386.s +++ b/src/runtime/sys_linux_386.s @@ -78,7 +78,7 @@ TEXT exit1<>(SB),NOSPLIT,$0 INT $3 // not reached RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVL wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_linux_amd64.s b/src/runtime/sys_linux_amd64.s index f0e58e11db..5431dea4fc 100644 --- a/src/runtime/sys_linux_amd64.s +++ b/src/runtime/sys_linux_amd64.s @@ -60,7 +60,7 @@ TEXT runtime·exit(SB),NOSPLIT,$0-4 SYSCALL RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-8 MOVQ wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_linux_arm.s b/src/runtime/sys_linux_arm.s index ca443b699f..e41afdb066 100644 --- a/src/runtime/sys_linux_arm.s +++ b/src/runtime/sys_linux_arm.s @@ -131,7 +131,7 @@ TEXT exit1<>(SB),NOSPLIT|NOFRAME,$0 MOVW $1003, R1 MOVW R0, (R1) // fail hard -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-4 MOVW wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_linux_arm64.s b/src/runtime/sys_linux_arm64.s index 1276c077d7..47e1718252 100644 --- a/src/runtime/sys_linux_arm64.s +++ b/src/runtime/sys_linux_arm64.s @@ -59,7 +59,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 SVC RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOVD wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_linux_mips64x.s b/src/runtime/sys_linux_mips64x.s index 0df2597993..315a6e4835 100644 --- a/src/runtime/sys_linux_mips64x.s +++ b/src/runtime/sys_linux_mips64x.s @@ -56,7 +56,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 SYSCALL RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOVV wait+0(FP), R1 // We're done using the stack. diff --git a/src/runtime/sys_linux_mipsx.s b/src/runtime/sys_linux_mipsx.s index 2207e9ab98..991b7ba60e 100644 --- a/src/runtime/sys_linux_mipsx.s +++ b/src/runtime/sys_linux_mipsx.s @@ -56,7 +56,7 @@ TEXT runtime·exit(SB),NOSPLIT,$0-4 UNDEF RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVW wait+0(FP), R1 // We're done using the stack. diff --git a/src/runtime/sys_linux_ppc64x.s b/src/runtime/sys_linux_ppc64x.s index dc3d89fae7..01d6c85597 100644 --- a/src/runtime/sys_linux_ppc64x.s +++ b/src/runtime/sys_linux_ppc64x.s @@ -55,7 +55,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 SYSCALL $SYS_exit_group RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOVD wait+0(FP), R1 // We're done using the stack. diff --git a/src/runtime/sys_linux_riscv64.s b/src/runtime/sys_linux_riscv64.s index a3da46d136..8b1b26b03b 100644 --- a/src/runtime/sys_linux_riscv64.s +++ b/src/runtime/sys_linux_riscv64.s @@ -61,7 +61,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 ECALL RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOV wait+0(FP), A0 // We're done using the stack. diff --git a/src/runtime/sys_linux_s390x.s b/src/runtime/sys_linux_s390x.s index 886add8b54..ca080b4225 100644 --- a/src/runtime/sys_linux_s390x.s +++ b/src/runtime/sys_linux_s390x.s @@ -52,7 +52,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0-4 SYSCALL RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT|NOFRAME,$0-8 MOVD wait+0(FP), R1 // We're done using the stack. diff --git a/src/runtime/sys_netbsd_386.s b/src/runtime/sys_netbsd_386.s index 8a33894892..6c5386bcf1 100644 --- a/src/runtime/sys_netbsd_386.s +++ b/src/runtime/sys_netbsd_386.s @@ -53,7 +53,7 @@ TEXT runtime·exit(SB),NOSPLIT,$-4 MOVL $0xf1, 0xf1 // crash RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVL wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_netbsd_amd64.s b/src/runtime/sys_netbsd_amd64.s index 02f5b4ba3b..c1cd95df14 100644 --- a/src/runtime/sys_netbsd_amd64.s +++ b/src/runtime/sys_netbsd_amd64.s @@ -122,7 +122,7 @@ TEXT runtime·exit(SB),NOSPLIT,$-8 MOVL $0xf1, 0xf1 // crash RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-8 MOVQ wait+0(FP), AX // We're done using the stack. diff --git a/src/runtime/sys_netbsd_arm.s b/src/runtime/sys_netbsd_arm.s index 3a763b2a6a..2422b0282e 100644 --- a/src/runtime/sys_netbsd_arm.s +++ b/src/runtime/sys_netbsd_arm.s @@ -56,7 +56,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0 MOVW.CS R8, (R8) RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-4 MOVW wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_netbsd_arm64.s b/src/runtime/sys_netbsd_arm64.s index 8a0496e807..6d2c31631d 100644 --- a/src/runtime/sys_netbsd_arm64.s +++ b/src/runtime/sys_netbsd_arm64.s @@ -114,7 +114,7 @@ TEXT runtime·exit(SB),NOSPLIT,$-8 MOVD $0, R0 // If we're still running, MOVD R0, (R0) // crash -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0-8 MOVD wait+0(FP), R0 // We're done using the stack. diff --git a/src/runtime/sys_openbsd2.go b/src/runtime/sys_openbsd2.go index 4d50b4f6b1..cba35cfee3 100644 --- a/src/runtime/sys_openbsd2.go +++ b/src/runtime/sys_openbsd2.go @@ -8,6 +8,7 @@ package runtime import ( "internal/abi" + "runtime/internal/atomic" "unsafe" ) @@ -250,7 +251,7 @@ func sigaltstack(new *stackt, old *stackt) { func sigaltstack_trampoline() // Not used on OpenBSD, but must be defined. -func exitThread(wait *uint32) { +func exitThread(wait *atomic.Uint32) { } //go:nosplit diff --git a/src/runtime/sys_openbsd_mips64.s b/src/runtime/sys_openbsd_mips64.s index f8ae8e7c30..bc392e4c54 100644 --- a/src/runtime/sys_openbsd_mips64.s +++ b/src/runtime/sys_openbsd_mips64.s @@ -24,7 +24,7 @@ TEXT runtime·exit(SB),NOSPLIT|NOFRAME,$0 MOVV R2, (R2) RET -// func exitThread(wait *uint32) +// func exitThread(wait *atomic.Uint32) TEXT runtime·exitThread(SB),NOSPLIT,$0 MOVV wait+0(FP), R4 // arg 1 - notdead MOVV $302, R2 // sys___threxit -- cgit v1.2.1 From aba57b07721cac39b4b7cf70f8dfbf9e2e299187 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 17 Oct 2022 17:38:29 -0700 Subject: [release-branch.go1.18] syscall, os/exec: reject environment variables containing NULs Check for and reject environment variables containing NULs. The conventions for passing environment variables to subprocesses cause most or all systems to interpret a NUL as a separator. The syscall package rejects environment variables containing a NUL on most systems, but erroneously did not do so on Windows. This causes an environment variable such as "FOO=a\x00BAR=b" to be interpreted as "FOO=a", "BAR=b". Check for and reject NULs in environment variables passed to syscall.StartProcess on Windows. Add a redundant check to os/exec as extra insurance. Updates #56284 Fixes #56327 Fixes CVE-2022-41716 Change-Id: I2950e2b0cb14ebd26e5629be1521858f66a7d4ae Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1609434 Run-TryBot: Damien Neil Reviewed-by: Tatiana Bradley Reviewed-by: Roland Shoemaker TryBot-Result: Security TryBots (cherry picked from commit 845accdebb2772c5344ed0c96df9910f3b02d741) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1617552 Run-TryBot: Tatiana Bradley Reviewed-by: Damien Neil Reviewed-on: https://go-review.googlesource.com/c/go/+/446915 Reviewed-by: Heschi Kreinick Run-TryBot: Matthew Dempsky TryBot-Result: Gopher Robot Reviewed-by: Tatiana Bradley --- src/os/exec/env_test.go | 19 +++++++++++++------ src/os/exec/exec.go | 19 +++++++++++++++---- src/os/exec/exec_test.go | 9 +++++++++ src/syscall/exec_windows.go | 20 +++++++++++++++----- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go index b5ac398c27..47b7c04705 100644 --- a/src/os/exec/env_test.go +++ b/src/os/exec/env_test.go @@ -11,9 +11,10 @@ import ( func TestDedupEnv(t *testing.T) { tests := []struct { - noCase bool - in []string - want []string + noCase bool + in []string + want []string + wantErr bool }{ { noCase: true, @@ -29,11 +30,17 @@ func TestDedupEnv(t *testing.T) { in: []string{"=a", "=b", "foo", "bar"}, want: []string{"=b", "foo", "bar"}, }, + { + // Filter out entries containing NULs. + in: []string{"A=a\x00b", "B=b", "C\x00C=c"}, + want: []string{"B=b"}, + wantErr: true, + }, } for _, tt := range tests { - got := dedupEnvCase(tt.noCase, tt.in) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Dedup(%v, %q) = %q; want %q", tt.noCase, tt.in, got, tt.want) + got, err := dedupEnvCase(tt.noCase, tt.in) + if !reflect.DeepEqual(got, tt.want) || (err != nil) != tt.wantErr { + t.Errorf("Dedup(%v, %q) = %q, %v; want %q, error:%v", tt.noCase, tt.in, got, err, tt.want, tt.wantErr) } } } diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index ecddee690d..056a7e07d7 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -422,10 +422,14 @@ func (c *Cmd) Start() error { return err } + env, err := dedupEnv(envv) + if err != nil { + return err + } c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{ Dir: c.Dir, Files: c.childFiles, - Env: addCriticalEnv(dedupEnv(envv)), + Env: addCriticalEnv(env), Sys: c.SysProcAttr, }) if err != nil { @@ -741,16 +745,23 @@ func minInt(a, b int) int { // dedupEnv returns a copy of env with any duplicates removed, in favor of // later values. // Items not of the normal environment "key=value" form are preserved unchanged. -func dedupEnv(env []string) []string { +// Items containing NUL characters are removed, and an error is returned along with +// the remaining values. +func dedupEnv(env []string) ([]string, error) { return dedupEnvCase(runtime.GOOS == "windows", env) } // dedupEnvCase is dedupEnv with a case option for testing. // If caseInsensitive is true, the case of keys is ignored. -func dedupEnvCase(caseInsensitive bool, env []string) []string { +func dedupEnvCase(caseInsensitive bool, env []string) ([]string, error) { + var err error out := make([]string, 0, len(env)) saw := make(map[string]int, len(env)) // key => index into out for _, kv := range env { + if strings.IndexByte(kv, 0) != -1 { + err = errors.New("exec: environment variable contains NUL") + continue + } k, _, ok := strings.Cut(kv, "=") if !ok { out = append(out, kv) @@ -766,7 +777,7 @@ func dedupEnvCase(caseInsensitive bool, env []string) []string { saw[k] = len(out) out = append(out, kv) } - return out + return out, err } // addCriticalEnv adds any critical environment variables that are required diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go index 1913ae81c8..0be8c6cc18 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -1029,6 +1029,15 @@ func TestDedupEnvEcho(t *testing.T) { } } +func TestEnvNULCharacter(t *testing.T) { + cmd := helperCommand(t, "echoenv", "FOO", "BAR") + cmd.Env = append(cmd.Env, "FOO=foo\x00BAR=bar") + out, err := cmd.CombinedOutput() + if err == nil { + t.Errorf("output = %q; want error", string(out)) + } +} + func TestString(t *testing.T) { echoPath, err := exec.LookPath("echo") if err != nil { diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go index 9d10d6a512..50892bee44 100644 --- a/src/syscall/exec_windows.go +++ b/src/syscall/exec_windows.go @@ -7,6 +7,7 @@ package syscall import ( + "internal/bytealg" "runtime" "sync" "unicode/utf16" @@ -115,12 +116,16 @@ func makeCmdLine(args []string) string { // the representation required by CreateProcess: a sequence of NUL // terminated strings followed by a nil. // Last bytes are two UCS-2 NULs, or four NUL bytes. -func createEnvBlock(envv []string) *uint16 { +// If any string contains a NUL, it returns (nil, EINVAL). +func createEnvBlock(envv []string) (*uint16, error) { if len(envv) == 0 { - return &utf16.Encode([]rune("\x00\x00"))[0] + return &utf16.Encode([]rune("\x00\x00"))[0], nil } length := 0 for _, s := range envv { + if bytealg.IndexByteString(s, 0) != -1 { + return nil, EINVAL + } length += len(s) + 1 } length += 1 @@ -135,7 +140,7 @@ func createEnvBlock(envv []string) *uint16 { } copy(b[i:i+1], []byte{0}) - return &utf16.Encode([]rune(string(b)))[0] + return &utf16.Encode([]rune(string(b)))[0], nil } func CloseOnExec(fd Handle) { @@ -400,12 +405,17 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle } } + envBlock, err := createEnvBlock(attr.Env) + if err != nil { + return 0, 0, err + } + pi := new(ProcessInformation) flags := sys.CreationFlags | CREATE_UNICODE_ENVIRONMENT | _EXTENDED_STARTUPINFO_PRESENT if sys.Token != 0 { - err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi) + err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, envBlock, dirp, &si.StartupInfo, pi) } else { - err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi) + err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, envBlock, dirp, &si.StartupInfo, pi) } if err != nil { return 0, 0, err -- cgit v1.2.1 From 156bf3dd36a9264f721dc98749c8899c559cca43 Mon Sep 17 00:00:00 2001 From: Gopher Robot Date: Tue, 1 Nov 2022 16:16:42 +0000 Subject: [release-branch.go1.18] go1.18.8 Change-Id: I89e791f1d6ae0984ba62bccef05886acbb10b2dd Reviewed-on: https://go-review.googlesource.com/c/go/+/446957 Run-TryBot: Gopher Robot Reviewed-by: Matthew Dempsky TryBot-Result: Gopher Robot Auto-Submit: Gopher Robot Reviewed-by: Heschi Kreinick --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index d5bd5caf57..aea94a5de8 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.18.7 \ No newline at end of file +go1.18.8 \ No newline at end of file -- cgit v1.2.1