Skip to content
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

runtime: memory corruption from stack-allocated defer on 32-bit #41795

Closed
aclements opened this issue Oct 5, 2020 · 5 comments
Closed

runtime: memory corruption from stack-allocated defer on 32-bit #41795

aclements opened this issue Oct 5, 2020 · 5 comments
Assignees
Labels
Milestone

Comments

@aclements
Copy link
Member

@aclements aclements commented Oct 5, 2020

On 32-bit architectures, a stack-allocated defer of a function with a 16-byte or smaller argument frame including a non-empty result area can corrupt memory if the deferred function grows the stack and is invoked during a panic. This has been possible since stack-allocated defers were implemented in Go 1.13.

This happens because the Go signature of runtime.call16 (the function used for reflectcall with argument frames 16 bytes or smaller) is wrong. Specifically, it is

func call16(fn, arg unsafe.Pointer, n, retoffset uint32)

but it should be

func call16(typ, fn, arg unsafe.Pointer, n, retoffset uint32)

As a result of this mismatch, the stack copier thought call16's third argument, arg, was a scalar rather than a pointer and thus would not adjust it even if it pointed into the stack. There's only one case where this happens: stack-allocated defers. All other uses of reflectcall pass a heap-allocated arg (which is kept reachable from other stack roots). If the deferred function grows the stack, then upon return, call16 would copy its results back to a stale arg frame that still points to where the stack used to be, potentially corrupting that memory.

It's basically impossible to write a unit test for this, but I confirmed it manually by stepping through the following example compiled for GOARCH=386:

package main

func main() {
	defer func() (x byte) {
		growStack(1000)
		return 42
	}()
	// Force the above defer to the stack.
	for i := 0; i < 1; i++ {
		defer func() {}()
	}
	// panic to invoke deferred functions reflectively.
	panic("x")
}

func growStack(n int) {
	if n != 0 {
		growStack(n - 1)
	}
}
$ GOARCH=386 go build x.go
$ gdb ./x
(gdb) br x.go:5
(gdb) br x.go:6
(gdb) r
Thread 1 "x" hit Breakpoint 1, main.main.func1 (x=<optimized out>) at /tmp/x.go:5
5			growStack(1000)
(gdb) print/x $getg().stack
$1 = {lo = 0x8442000, hi = 0x8442800}
# ^ Stack prior to call16
(gdb) c
Thread 1 "x" hit Breakpoint 2, main.main.func1 (x=<optimized out>) at /tmp/x.go:6
6			return 42
(gdb) print/x $getg().stack
$2 = {lo = 0x8476000, hi = 0x847a000}
# ^ Stack after stack move
(gdb) br 'runtime.reflectcallmove'
Breakpoint 3 at 0x8054140: file /home/austin/go.dev/src/runtime/mbarrier.go, line 226.
(gdb) c
Thread 1 "x" hit Breakpoint 3, runtime.reflectcallmove (typ=0x0, dst=0x84427c4, src=0x8479f18, size=0)
(gdb)

In reflectcallmove, the dst argument refers to the stack's address from before the stack move, which could have been reallocated.

@aclements aclements added the NeedsFix label Oct 5, 2020
@aclements aclements added this to the Go1.16 milestone Oct 5, 2020
@aclements aclements self-assigned this Oct 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259338 mentions this issue: runtime: correct signature of call16

@aclements
Copy link
Member Author

@aclements aclements commented Oct 5, 2020

@gopherbot, please open backports to Go 1.15 and 1.14.

This can cause random memory corruption on 32-bit architectures. The circumstances for it happening are incredibly narrow, but the fix is also trivial.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Backport issue(s) opened: #41796 (for 1.14), #41797 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot closed this in 9dc65d7 Oct 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259597 mentions this issue: [release-branch.go1.14] runtime: correct signature of call16

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259598 mentions this issue: [release-branch.go1.15] runtime: correct signature of call16

gopherbot pushed a commit that referenced this issue Oct 14, 2020
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see #17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates #41795.
Fixes #41797.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259598
gopherbot pushed a commit that referenced this issue Oct 14, 2020
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see #17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates #41795.
Fixes #41796.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259597
claucece added a commit to claucece/go that referenced this issue Oct 22, 2020
The signature of call16 is currently missing the "typ" parameter. This
CL fixes this. This wasn't caught by vet because call16 is defined by
macro expansion (see golang#17544), and we didn't notice the mismatch with
the other call* functions because call16 is defined only on 32-bit
architectures and lives alone in stubs32.go.

Unfortunately, this means its GC signature is also wrong: the "arg"
parameter is treated as a scalar rather than a pointer, so GC won't
trace it and stack copying won't adjust it. This turns out to matter
in exactly one case right now: on 32-bit architectures (which are the
only architectures where call16 is defined), a stack-allocated defer
of a function with a 16-byte or smaller argument frame including a
non-empty result area can corrupt memory if the deferred function
grows the stack and is invoked during a panic. Whew. All other current
uses of reflectcall pass a heap-allocated "arg" frame (which happens
to be reachable from other stack roots, so tracing isn't a problem).

Curiously, in 2016, the signatures of all call* functions were wrong
in exactly this way. CL 31654 fixed all of them in stubs.go, but
missed the one in stubs32.go.

Updates golang#41795.
Fixes golang#41797.

Change-Id: I31e3c0df201f79ee5707eeb8dc4ff0d13fc10ada
Reviewed-on: https://go-review.googlesource.com/c/go/+/259338
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/259598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.