cmd/compile: check for negative upper bound to IsSliceInBounds

IsSliceInBounds(x, y) asserts that y is not negative, but
there were cases where this is not true.  Change code
generation to ensure that this is true when it's not obviously
true.  Prove phase cleans a few of these out.

With this change the compiler text section is 0.06% larger,
that is, not very much.  Benchmarking still TBD, may need
to wait for access to a benchmarking box (next week).

Also corrected run.go to handle '?' in -update_errors output.

Fixes #28797.

Change-Id: Ia8af90bc50a91ae6e934ef973def8d3f398fac7b
Reviewed-on: https://go-review.googlesource.com/c/152477
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/cmd/compile/internal/gc/ssa.go

@@ -4022,6 +4022,18 @@ func (s *state) boundsCheck(idx, len *ssa.Value) {
 	s.check(cmp, panicindex)
 }
 
+func couldBeNegative(v *ssa.Value) bool {
+	switch v.Op {
+	case ssa.OpSliceLen, ssa.OpSliceCap, ssa.OpStringLen:
+		return false
+	case ssa.OpConst64:
+		return v.AuxInt < 0
+	case ssa.OpConst32:
+		return int32(v.AuxInt) < 0
+	}
+	return true
+}
+
 // sliceBoundsCheck generates slice bounds checking code. Checks if 0 <= idx <= len, branches to exit if not.
 // Starts a new block on return.
 // idx and len are already converted to full int width.
@@ -4029,6 +4041,15 @@ func (s *state) sliceBoundsCheck(idx, len *ssa.Value) {
 	if Debug['B'] != 0 {
 		return
 	}
+	if couldBeNegative(len) {
+		// OpIsSliceInBounds requires second arg not negative; if it's not obviously true, must check.
+		cmpop := ssa.OpGeq64
+		if len.Type.Size() == 4 {
+			cmpop = ssa.OpGeq32
+		}
+		cmp := s.newValue2(cmpop, types.Types[TBOOL], len, s.zeroVal(len.Type))
+		s.check(cmp, panicslice)
+	}
 
 	// bounds check
 	cmp := s.newValue2(ssa.OpIsSliceInBounds, types.Types[TBOOL], idx, len)

test/fixedbugs/issue28797.go

+// run
+
+// Copyright 2018 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 (
+	"fmt"
+)
+
+// test expects f to panic, but not to run out of memory,
+// which is a non-panic fatal error.  OOM results from failure
+// to properly check negative limit.
+func test(f func()) {
+	defer func() {
+		r := recover()
+		if r == nil {
+			panic("panic wasn't recoverable")
+		}
+	}()
+	f()
+}
+
+//go:noinline
+func id(x int) int {
+	return x
+}
+
+func main() {
+	test(foo)
+	test(bar)
+}
+
+func foo() {
+	b := make([]byte, 0)
+	b = append(b, 1)
+	id(len(b))
+	id(len(b) - 2)
+	s := string(b[1 : len(b)-2])
+	fmt.Println(s)
+}
+
+func bar() {
+	b := make([]byte, 1)
+	b = append(b, 1)
+	i := id(-1)
+	if i < len(b) { // establish value is not too large.
+		s := string(b[1:i]) // should check for negative also.
+		fmt.Println(s)
+	}
+}

test/prove.go

@@ -62,7 +62,7 @@ func f1c(a []int, i int64) int {
 }
 
 func f2(a []int) int {
-	for i := range a { // ERROR "Induction variable: limits \[0,\?\), increment 1"
+	for i := range a { // ERROR "Induction variable: limits \[0,\?\), increment 1$"
 		a[i+1] = i
 		a[i+1] = i // ERROR "Proved IsInBounds$"
 	}
@@ -269,7 +269,7 @@ func f11b(a []int, i int) {
 
 func f11c(a []int, i int) {
 	useSlice(a[:i])
-	useSlice(a[:i]) // ERROR "Proved IsSliceInBounds$"
+	useSlice(a[:i]) // ERROR "Proved Geq64$" "Proved IsSliceInBounds$"
 }
 
 func f11d(a []int, i int) {
@@ -464,12 +464,12 @@ func f16(s []int) []int {
 }
 
 func f17(b []int) {
-	for i := 0; i < len(b); i++ { // ERROR "Induction variable: limits \[0,\?\), increment 1"
+	for i := 0; i < len(b); i++ { // ERROR "Induction variable: limits \[0,\?\), increment 1$"
 		// This tests for i <= cap, which we can only prove
 		// using the derived relation between len and cap.
 		// This depends on finding the contradiction, since we
 		// don't query this condition directly.
-		useSlice(b[:i]) // ERROR "Proved IsSliceInBounds$"
+		useSlice(b[:i]) // ERROR "Proved Geq64$" "Proved IsSliceInBounds$"
 	}
 }
 
@@ -579,18 +579,18 @@ func fence4(x, y int64) {
 func trans1(x, y int64) {
 	if x > 5 {
 		if y > x {
-			if y > 2 { // ERROR "Proved Greater64"
+			if y > 2 { // ERROR "Proved Greater64$"
 				return
 			}
 		} else if y == x {
-			if y > 5 { // ERROR "Proved Greater64"
+			if y > 5 { // ERROR "Proved Greater64$"
 				return
 			}
 		}
 	}
 	if x >= 10 {
 		if y > x {
-			if y > 10 { // ERROR "Proved Greater64"
+			if y > 10 { // ERROR "Proved Greater64$"
 				return
 			}
 		}
@@ -624,7 +624,7 @@ func natcmp(x, y []uint) (r int) {
 	}
 
 	i := m - 1
-	for i > 0 && // ERROR "Induction variable: limits \(0,\?\], increment 1"
+	for i > 0 && // ERROR "Induction variable: limits \(0,\?\], increment 1$"
 		x[i] == // ERROR "Proved IsInBounds$"
 			y[i] { // ERROR "Proved IsInBounds$"
 		i--
@@ -686,7 +686,7 @@ func range2(b [][32]int) {
 		if i < len(b) {    // ERROR "Proved Less64$"
 			println("x")
 		}
-		if i >= 0 { // ERROR "Proved Geq64"
+		if i >= 0 { // ERROR "Proved Geq64$"
 			println("x")
 		}
 	}

test/run.go

@@ -1212,7 +1212,7 @@ func (t *test) updateErrors(out, file string) {
 		msg := errStr[colon2+2:]
 		msg = strings.Replace(msg, file, base, -1) // normalize file mentions in error itself
 		msg = strings.TrimLeft(msg, " \t")
-		for _, r := range []string{`\`, `*`, `+`, `[`, `]`, `(`, `)`} {
+		for _, r := range []string{`\`, `*`, `+`, `?`, `[`, `]`, `(`, `)`} {
 			msg = strings.Replace(msg, r, `\`+r, -1)
 		}
 		msg = strings.Replace(msg, `"`, `.`, -1)