From 627798db4e42e4fd3f80a4149cbfafff80b304f3 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Tue, 28 Mar 2017 16:03:24 -0400 Subject: [PATCH] reflect: fix out-of-bounds pointers calling no-result method reflect.callReflect heap-allocates a stack frame and then constructs pointers to the arguments and result areas of that frame. However, if there are no results, the results pointer will point past the end of the frame allocation. If there are also no arguments, the arguments pointer will also point past the end of the frame allocation. If the GC observes either these pointers, it may panic. Fix this by not constructing these pointers if these areas of the frame are empty. This adds a test of calling no-argument/no-result methods via reflect, since nothing in std did this before. However, it's quite difficult to demonstrate the actual failure because it depends on both exact allocation patterns and on GC scanning the goroutine's stack while inside one of the typedmemmovepartial calls. I also audited other uses of typedmemmovepartial and memclrNoHeapPointers in reflect, since these are the most susceptible to this. These appear to be the only two cases that can construct out-of-bounds arguments to these functions. Fixes #19724. Change-Id: I4b83c596b5625dc4ad0567b1e281bad4faef972b Reviewed-on: https://go-review.googlesource.com/38736 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/reflect/all_test.go | 35 ++++++++++++++++++++++++++++++++++- src/reflect/value.go | 26 ++++++++++++++++---------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go index 0b81c28a22..382ad6be2a 100644 --- a/src/reflect/all_test.go +++ b/src/reflect/all_test.go @@ -1681,6 +1681,11 @@ func (p Point) GCMethod(k int) int { } // This will be index 3. +func (p Point) NoArgs() { + // Exercise no-argument/no-result paths. +} + +// This will be index 4. func (p Point) TotalDist(points ...Point) int { tot := 0 for _, q := range points { @@ -1709,6 +1714,15 @@ func TestMethod(t *testing.T) { t.Errorf("Type MethodByName returned %d; want 275", i) } + m, ok = TypeOf(p).MethodByName("NoArgs") + if !ok { + t.Fatalf("method by name failed") + } + n := len(m.Func.Call([]Value{ValueOf(p)})) + if n != 0 { + t.Errorf("NoArgs returned %d values; want 0", n) + } + i = TypeOf(&p).Method(1).Func.Call([]Value{ValueOf(&p), ValueOf(12)})[0].Int() if i != 300 { t.Errorf("Pointer Type Method returned %d; want 300", i) @@ -1723,6 +1737,15 @@ func TestMethod(t *testing.T) { t.Errorf("Pointer Type MethodByName returned %d; want 325", i) } + m, ok = TypeOf(&p).MethodByName("NoArgs") + if !ok { + t.Fatalf("method by name failed") + } + n = len(m.Func.Call([]Value{ValueOf(&p)})) + if n != 0 { + t.Errorf("NoArgs returned %d values; want 0", n) + } + // Curried method of value. tfunc := TypeOf((func(int) int)(nil)) v := ValueOf(p).Method(1) @@ -1741,6 +1764,8 @@ func TestMethod(t *testing.T) { if i != 375 { t.Errorf("Value MethodByName returned %d; want 375", i) } + v = ValueOf(p).MethodByName("NoArgs") + v.Call(nil) // Curried method of pointer. v = ValueOf(&p).Method(1) @@ -1759,6 +1784,8 @@ func TestMethod(t *testing.T) { if i != 425 { t.Errorf("Pointer Value MethodByName returned %d; want 425", i) } + v = ValueOf(&p).MethodByName("NoArgs") + v.Call(nil) // Curried method of interface value. // Have to wrap interface value in a struct to get at it. @@ -1808,6 +1835,9 @@ func TestMethodValue(t *testing.T) { if i != 275 { t.Errorf("Value MethodByName returned %d; want 275", i) } + v = ValueOf(p).MethodByName("NoArgs") + ValueOf(v.Interface()).Call(nil) + v.Interface().(func())() // Curried method of pointer. v = ValueOf(&p).Method(1) @@ -1826,6 +1856,9 @@ func TestMethodValue(t *testing.T) { if i != 325 { t.Errorf("Pointer Value MethodByName returned %d; want 325", i) } + v = ValueOf(&p).MethodByName("NoArgs") + ValueOf(v.Interface()).Call(nil) + v.Interface().(func())() // Curried method of pointer to pointer. pp := &p @@ -1881,7 +1914,7 @@ func TestVariadicMethodValue(t *testing.T) { // Curried method of value. tfunc := TypeOf((func(...Point) int)(nil)) - v := ValueOf(p).Method(3) + v := ValueOf(p).Method(4) if tt := v.Type(); tt != tfunc { t.Errorf("Variadic Method Type is %s; want %s", tt, tfunc) } diff --git a/src/reflect/value.go b/src/reflect/value.go index 44a91f06c6..dd4bafac24 100644 --- a/src/reflect/value.go +++ b/src/reflect/value.go @@ -630,8 +630,11 @@ func callMethod(ctxt *methodValue, frame unsafe.Pointer) { args := framePool.Get().(unsafe.Pointer) // Copy in receiver and rest of args. + // Avoid constructing out-of-bounds pointers if there are no args. storeRcvr(rcvr, args) - typedmemmovepartial(frametype, unsafe.Pointer(uintptr(args)+ptrSize), frame, ptrSize, argSize-ptrSize) + if argSize-ptrSize > 0 { + typedmemmovepartial(frametype, unsafe.Pointer(uintptr(args)+ptrSize), frame, ptrSize, argSize-ptrSize) + } // Call. call(frametype, fn, args, uint32(frametype.size), uint32(retOffset)) @@ -641,15 +644,18 @@ func callMethod(ctxt *methodValue, frame unsafe.Pointer) { // a receiver) is different from the layout of the fn call, which has // a receiver. // Ignore any changes to args and just copy return values. - callerRetOffset := retOffset - ptrSize - if runtime.GOARCH == "amd64p32" { - callerRetOffset = align(argSize-ptrSize, 8) - } - typedmemmovepartial(frametype, - unsafe.Pointer(uintptr(frame)+callerRetOffset), - unsafe.Pointer(uintptr(args)+retOffset), - retOffset, - frametype.size-retOffset) + // Avoid constructing out-of-bounds pointers if there are no return values. + if frametype.size-retOffset > 0 { + callerRetOffset := retOffset - ptrSize + if runtime.GOARCH == "amd64p32" { + callerRetOffset = align(argSize-ptrSize, 8) + } + typedmemmovepartial(frametype, + unsafe.Pointer(uintptr(frame)+callerRetOffset), + unsafe.Pointer(uintptr(args)+retOffset), + retOffset, + frametype.size-retOffset) + } // This is untyped because the frame is really a stack, even // though it's a heap object. -- 2.48.1