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

Jump to bottom

runtime: append does not fail when length overflows #29190

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

runtime: append does not fail when length overflows #29190

icza opened this issue Dec 12, 2018 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.13

Comments

@icza
Copy link

icza 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
Copy link
Contributor

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

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 12, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Dec 12, 2018
@martisch
Copy link
Contributor

martisch commented Dec 12, 2018 •
edited

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):

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

Line 2457 in d70b0ec

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

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

Line 2649 in d70b0ec

// 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
Copy link
Contributor

dgryski 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
Copy link
Contributor

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
Copy link

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

@martisch martisch mentioned this issue Jan 2, 2019
Closed
@golang golang locked and limited conversation to collaborators Dec 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@martisch @dgryski @ianlancetaylor @icza @gopherbot