cmd/compile: don't move nil checks across a VarDef

We need to make sure that there's no possible faulting
instruction between a VarDef and that variable being
fully initialized. If there was, then anything scanning
the stack during the handling of that fault will see
a live but uninitialized variable on the stack.

If we have:

  NilCheck p
  VarDef x
  x = *p

We can't rewrite that to

  VarDef x
  NilCheck p
  x = *p

Particularly, even though *p faults on p==nil, we still
have to do the explicit nil check before the VarDef.

Fixes #32288

Change-Id: Ib8b88e6a5af3bf6f238ff5491ac86f53f3cf9fc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/179239
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>

diff --git a/src/cmd/compile/internal/ssa/nilcheck.go b/src/cmd/compile/internal/ssa/nilcheck.go
index 925f55234b2489ec22296202a5853a326e942a4d..54c9c9d7ded6935ebc7a54c7be576238e90fbe0c 100644 (file)
--- a/src/cmd/compile/internal/ssa/nilcheck.go
+++ b/src/cmd/compile/internal/ssa/nilcheck.go
@@ -219,9 +219,31 @@ func nilcheckelim2(f *Func) {
                                continue
                        }
                        if v.Type.IsMemory() || v.Type.IsTuple() && v.Type.FieldType(1).IsMemory() {
-                               if v.Op == OpVarDef || v.Op == OpVarKill || v.Op == OpVarLive {
+                               if v.Op == OpVarKill || v.Op == OpVarLive || (v.Op == OpVarDef && !v.Aux.(GCNode).Typ().HasHeapPointer()) {
                                        // These ops don't really change memory.
                                        continue
+                                       // Note: OpVarDef requires that the defined variable not have pointers.
+                                       // We need to make sure that there's no possible faulting
+                                       // instruction between a VarDef and that variable being
+                                       // fully initialized. If there was, then anything scanning
+                                       // the stack during the handling of that fault will see
+                                       // a live but uninitialized pointer variable on the stack.
+                                       //
+                                       // If we have:
+                                       //
+                                       //   NilCheck p
+                                       //   VarDef x
+                                       //   x = *p
+                                       //
+                                       // We can't rewrite that to
+                                       //
+                                       //   VarDef x
+                                       //   NilCheck p
+                                       //   x = *p
+                                       //
+                                       // Particularly, even though *p faults on p==nil, we still
+                                       // have to do the explicit nil check before the VarDef.
+                                       // See issue #32288.
                                }
                                // This op changes memory.  Any faulting instruction after v that
                                // we've recorded in the unnecessary map is now obsolete.

diff --git a/test/fixedbugs/issue32288.go b/test/fixedbugs/issue32288.go
new file mode 100644 (file)
index 0000000..91c930c
--- /dev/null
+++ b/test/fixedbugs/issue32288.go
@@ -0,0 +1,48 @@
+// run
+
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+type T struct {
+       s   [1]string
+       pad [16]uintptr
+}
+
+//go:noinline
+func f(t *int, p *int) []T {
+       var res []T
+       for {
+               var e *T
+               res = append(res, *e)
+       }
+}
+
+func main() {
+       defer func() {
+               useStack(100) // force a stack copy
+               // We're expecting a panic.
+               // The bug in this issue causes a throw, which this recover() will not squash.
+               recover()
+       }()
+       junk() // fill the stack with invalid pointers
+       f(nil, nil)
+}
+
+func useStack(n int) {
+       if n == 0 {
+               return
+       }
+       useStack(n - 1)
+}
+
+//go:noinline
+func junk() uintptr {
+       var a [128]uintptr // 1k of bad pointers on the stack
+       for i := range a {
+               a[i] = 0xaa
+       }
+       return a[12]
+}