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 <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/reflect/all_test.go b/src/reflect/all_test.go
index 0b81c28a22afaebceaf2c42cf0909541665aac50..382ad6be2aa779b72439db409f864cec281ea400 100644 (file)
--- 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 44a91f06c656ed62301451c3458c9735acf6b0da..dd4bafac2429979d089694b8b414a41b3c7a7904 100644 (file)
--- 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.