Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: append does not fail when length overflows #29190

Closed
icza opened this issue Dec 12, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@icza
Copy link

commented Dec 12, 2018

What version of Go are you using (go version)?

$ go version
go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/icza/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/icza/gows"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build173735906=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run the following app:

package main

import (
	"fmt"
	"math"
)

func main() {
	s := make([]struct{}, math.MaxInt64-2)
	fmt.Println(len(s), cap(s))
	for i := 0; i < 5; i++ {
		s = append(s, struct{}{})
		fmt.Println(len(s), cap(s))
	}
}

A slightly modified version is available and reproduces the error on the Go Playground: https://play.golang.org/p/AUKUBmQld_e

On the Playground math.MaxInt32 is used instead of math.MaxInt64.

What did you expect to see?

A panic saying:

growslice: cap out of range

What did you see instead?

The app did not fail. Capacity stops growing when it reaches MaxInt64 on 64-bit architecture, and MaxInt32 on 32-bit architectures, but length overflows and becomes negative.

9223372036854775805 9223372036854775805
9223372036854775806 9223372036854775806
9223372036854775807 9223372036854775807
-9223372036854775808 9223372036854775807
-9223372036854775807 9223372036854775807
-9223372036854775806 9223372036854775807

This is in clear violation of the Spec: Length and capacity:

At any time the following relationship holds:
0 <= len(s) <= cap(s)

as length becomes negative (and thus less than 0).

This report originates from this Stackoverflow question: https://stackoverflow.com/questions/53743099/behavior-of-append-when-appending-item-to-a-max-size-slice

@ianlancetaylor ianlancetaylor changed the title append() does not fail when length overflows runtime: append does not fail when length overflows Dec 12, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

With Go 1.6 and gccgo I get the following crash. With Go 1.7 and later the program succeeds incorrectly.

I'm going to mark this a release blocker for 1.13, but of course if we have a fix for 1.12 that would be nice.

9223372036854775805 9223372036854775805
9223372036854775806 9223372036854775806
9223372036854775807 9223372036854775807
panic: runtime error: growslice: cap out of range

goroutine 1 [running]:
panic
	../../../gccgo3/libgo/go/runtime/panic.go:588
main.main
	/home/iant/foo.go:12
exit status 2
@martisch

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

From a quick look and test I think we can fix the issue by changing the comparison from INT to UINT here (and assuming no one will ever write code appending more than MaxInt arguments in a single append call - which likely would result in the parser running out of memory first):

cmp := s.newValue2(s.ssaOp(OGT, types.Types[TINT]), types.Types[TBOOL], nl, c)

With that change I get:

9223372036854775805 9223372036854775805
9223372036854775806 9223372036854775806
9223372036854775807 9223372036854775807
panic: runtime error: growslice: cap out of range

goroutine 1 [running]:
main.main()
	/Users/martisch/test/main.go:71 +0x222
exit status 2

The

// if uint(n) > uint(cap(s))
code for appending lists already does a UINT comparison.

Will send a CL for go1.12 after some more testing and writing a test case within the next day.

@martisch martisch self-assigned this Dec 12, 2018

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

and assuming no one will ever write code appending MaxInt/2 arguments in a single append call - which likely would result in the parser running out of memory first

Does that include a = append(a, b...) where len(b) > MaxInt/2 ?

@martisch

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Sorry I meant MaxInt before (updated the comment). a = append(a, b...) can only overflow if len(a)+len(b)>MaxUInt as that code path already does the comparison check with UInt. If the element sizes are larger than zero then it also should produce an out of memory error before it gets to the append (unless some maybe if unsafe or mmap trickery is involved). When I last tried I could not allocate a maxInt byte slice on either 32bit or 64bit without a panic in make.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 13, 2018

Change https://golang.org/cl/154037 mentions this issue: cmd/compile: fix length overflow when appending elements to a slice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.