runtime: don't crash on nil pointers in checkptrAlignment

Ironically, checkptrAlignment had a latent case of bad pointer
arithmetic: if ptr is nil, then `add(ptr, size-1)` might produce an
illegal pointer value.

The fix is to simply check for nil at the top of checkptrAlignment,
and short-circuit if so.

This CL also adds a more explicit bounds check in checkptrStraddles,
rather than relying on `add(ptr, size-1)` to wrap around. I don't
think this is necessary today, but it seems prudent to be careful.

Fixes #47430.

Change-Id: I5c50b2f7f41415dbebbd803e1b8e7766ca95e1fd
Reviewed-on: https://go-review.googlesource.com/c/go/+/338029
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>

3 files changed, 45 insertions, 3 deletions

diff --git a/src/runtime/checkptr.go b/src/runtime/checkptr.go
index d42950844b..2d4afd5cf6 100644
--- a/src/runtime/checkptr.go
+++ b/src/runtime/checkptr.go
@@ -7,6 +7,11 @@ package runtime
 import "unsafe"
 
 func checkptrAlignment(p unsafe.Pointer, elem *_type, n uintptr) {
+	// nil pointer is always suitably aligned (#47430).
+	if p == nil {
+		return
+	}
+
 	// Check that (*[n]elem)(p) is appropriately aligned.
 	// Note that we allow unaligned pointers if the types they point to contain
 	// no pointers themselves. See issue 37298.
@@ -29,10 +34,12 @@ func checkptrStraddles(ptr unsafe.Pointer, size uintptr) bool {
 		return false
 	}
 
-	end := add(ptr, size-1)
-	if uintptr(end) < uintptr(ptr) {
+	// Check that add(ptr, size-1) won't overflow. This avoids the risk
+	// of producing an illegal pointer value (assuming ptr is legal).
+	if uintptr(ptr) >= -(size - 1) {
 		return true
 	}
+	end := add(ptr, size-1)
 
 	// TODO(mdempsky): Detect when [ptr, end] contains Go allocations,
 	// but neither ptr nor end point into one themselves.
diff --git a/src/runtime/checkptr_test.go b/src/runtime/checkptr_test.go
index 2a5c364e97..d5dd101adb 100644
--- a/src/runtime/checkptr_test.go
+++ b/src/runtime/checkptr_test.go
@@ -26,6 +26,7 @@ func TestCheckPtr(t *testing.T) {
 	}{
 		{"CheckPtrAlignmentPtr", "fatal error: checkptr: misaligned pointer conversion\n"},
 		{"CheckPtrAlignmentNoPtr", ""},
+		{"CheckPtrAlignmentNilPtr", ""},
 		{"CheckPtrArithmetic", "fatal error: checkptr: pointer arithmetic result points to invalid allocation\n"},
 		{"CheckPtrArithmetic2", "fatal error: checkptr: pointer arithmetic result points to invalid allocation\n"},
 		{"CheckPtrSize", "fatal error: checkptr: converted pointer straddles multiple allocations\n"},
diff --git a/src/runtime/testdata/testprog/checkptr.go b/src/runtime/testdata/testprog/checkptr.go
index f76b64ad96..9c5561396e 100644
--- a/src/runtime/testdata/testprog/checkptr.go
+++ b/src/runtime/testdata/testprog/checkptr.go
@@ -4,11 +4,16 @@
 
 package main
 
-import "unsafe"
+import (
+	"runtime"
+	"time"
+	"unsafe"
+)
 
 func init() {
 	register("CheckPtrAlignmentNoPtr", CheckPtrAlignmentNoPtr)
 	register("CheckPtrAlignmentPtr", CheckPtrAlignmentPtr)
+	register("CheckPtrAlignmentNilPtr", CheckPtrAlignmentNilPtr)
 	register("CheckPtrArithmetic", CheckPtrArithmetic)
 	register("CheckPtrArithmetic2", CheckPtrArithmetic2)
 	register("CheckPtrSize", CheckPtrSize)
@@ -29,6 +34,35 @@ func CheckPtrAlignmentPtr() {
 	sink2 = (**int64)(unsafe.Pointer(uintptr(p) + 1))
 }
 
+// CheckPtrAlignmentNilPtr tests that checkptrAlignment doesn't crash
+// on nil pointers (#47430).
+func CheckPtrAlignmentNilPtr() {
+	var do func(int)
+	do = func(n int) {
+		// Inflate the stack so runtime.shrinkstack gets called during GC
+		if n > 0 {
+			do(n - 1)
+		}
+
+		var p unsafe.Pointer
+		_ = (*int)(p)
+	}
+
+	go func() {
+		for {
+			runtime.GC()
+		}
+	}()
+
+	go func() {
+		for i := 0; ; i++ {
+			do(i % 1024)
+		}
+	}()
+
+	time.Sleep(time.Second)
+}
+
 func CheckPtrArithmetic() {
 	var x int
 	i := uintptr(unsafe.Pointer(&x))