[go] cmd/compile: don't assume pointer of a slice is nil

1 view

Skip to first unread message

Keith Randall (Gerrit)

unread,

Mar 28, 2023, 3:55:47 PM3/28/23

to Keith Randall, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Matthew Dempsky, Ian Lance Taylor, Gopher Robot, golang-co...@googlegroups.com

Keith Randall submitted this change.

View Change



1 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.

Approvals:
  Matthew Dempsky: Looks good to me, approved
  Keith Randall: Looks good to me, but someone else must approve
  Keith Randall: Run TryBots
  Gopher Robot: TryBots succeeded

cmd/compile: don't assume pointer of a slice is non-nil

unsafe.SliceData can return pointers which are nil. That function gets
lowered to the SSA OpSlicePtr, which the compiler assumes is non-nil.
This used to be the case as OpSlicePtr was only used in situations
where the bounds check already passed. But with unsafe.SliceData that
is no longer the case.

There are situations where we know it is nil. Use Bounded() to
indicate that.

I looked through all the uses of OSPTR and added SetBounded where it
made sense. Most OSPTR results are passed directly to runtime calls
(e.g. memmove), so even if we know they are non-nil that info isn't
helpful.

Fixes #59293

Change-Id: I437a15330db48e0082acfb1f89caf8c56723fc51
Reviewed-on: https://go-review.googlesource.com/c/go/+/479896
Reviewed-by: Matthew Dempsky <mdem...@google.com>
Reviewed-by: Keith Randall <k...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Keith Randall <k...@golang.org>
---
M src/cmd/compile/internal/ir/node.go
M src/cmd/compile/internal/ssagen/ssa.go
M src/cmd/compile/internal/walk/convert.go
M src/cmd/compile/internal/walk/range.go
A test/fixedbugs/issue59293.go
5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/src/cmd/compile/internal/ir/node.go b/src/cmd/compile/internal/ir/node.go
index ad25b9f..bdc40a8 100644
--- a/src/cmd/compile/internal/ir/node.go
+++ b/src/cmd/compile/internal/ir/node.go
@@ -292,7 +292,7 @@
 	OEFACE         // itable and data words of an empty-interface value.
 	OITAB          // itable word of an interface value.
 	OIDATA         // data word of an interface value in X
-	OSPTR          // base pointer of a slice or string.
+	OSPTR          // base pointer of a slice or string. Bounded==1 means known non-nil.
 	OCFUNC         // reference to c function pointer (not go func value)
 	OCHECKNIL      // emit code to ensure pointer/interface not nil
 	ORESULT        // result of a function call; Xoffset is stack offset
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index e49ba5e..a376049 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -3199,7 +3199,10 @@
 		n := n.(*ir.UnaryExpr)
 		a := s.expr(n.X)
 		if n.X.Type().IsSlice() {
-			return s.newValue1(ssa.OpSlicePtr, n.Type(), a)
+			if n.Bounded() {
+				return s.newValue1(ssa.OpSlicePtr, n.Type(), a)
+			}
+			return s.newValue1(ssa.OpSlicePtrUnchecked, n.Type(), a)
 		} else {
 			return s.newValue1(ssa.OpStringPtr, n.Type(), a)
 		}
diff --git a/src/cmd/compile/internal/walk/convert.go b/src/cmd/compile/internal/walk/convert.go
index 07ddd04..bfa0c54 100644
--- a/src/cmd/compile/internal/walk/convert.go
+++ b/src/cmd/compile/internal/walk/convert.go
@@ -281,7 +281,9 @@
 
 		// Copy from the static string data to the [n]byte.
 		if len(sc) > 0 {
-			as := ir.NewAssignStmt(base.Pos, ir.NewStarExpr(base.Pos, p), ir.NewStarExpr(base.Pos, typecheck.ConvNop(ir.NewUnaryExpr(base.Pos, ir.OSPTR, s), t.PtrTo())))
+			sptr := ir.NewUnaryExpr(base.Pos, ir.OSPTR, s)
+			sptr.SetBounded(true)
+			as := ir.NewAssignStmt(base.Pos, ir.NewStarExpr(base.Pos, p), ir.NewStarExpr(base.Pos, typecheck.ConvNop(sptr, t.PtrTo())))
 			appendWalkStmt(init, as)
 		}
 
diff --git a/src/cmd/compile/internal/walk/range.go b/src/cmd/compile/internal/walk/range.go
index e20ffc2..1d757a6 100644
--- a/src/cmd/compile/internal/walk/range.go
+++ b/src/cmd/compile/internal/walk/range.go
@@ -193,6 +193,7 @@
 		// Pointer to current iteration position. Start on entry to the loop
 		// with the pointer in hu.
 		ptr := ir.NewUnaryExpr(base.Pos, ir.OSPTR, hs)
+		ptr.SetBounded(true)
 		huVal := ir.NewConvExpr(base.Pos, ir.OCONVNOP, types.Types[types.TUNSAFEPTR], ptr)
 		huVal = ir.NewConvExpr(base.Pos, ir.OCONVNOP, types.Types[types.TUINTPTR], huVal)
 		hu := typecheck.Temp(types.Types[types.TUINTPTR])
diff --git a/test/fixedbugs/issue59293.go b/test/fixedbugs/issue59293.go
new file mode 100644
index 0000000..1f05fe9
--- /dev/null
+++ b/test/fixedbugs/issue59293.go
@@ -0,0 +1,28 @@
+// run
+
+// Copyright 2023 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
+
+import "unsafe"
+
+//go:noinline
+func f(x []byte) bool {
+	return unsafe.SliceData(x) != nil
+}
+
+//go:noinline
+func g(x string) bool {
+	return unsafe.StringData(x) != nil
+}
+
+func main() {
+	if f(nil) {
+		panic("bad f")
+	}
+	if g("") {
+		panic("bad g")
+	}
+}

To view, visit change 479896. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages