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

Inlining prevents a slice panic #28817

Closed
thecrypticace opened this issue Nov 15, 2018 · 3 comments
Closed

Inlining prevents a slice panic #28817

thecrypticace opened this issue Nov 15, 2018 · 3 comments

Comments

@thecrypticace
Copy link

@thecrypticace thecrypticace commented Nov 15, 2018

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

$ go version
go version go1.11.2 darwin/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="/Users/jordanpittman/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jordanpittman/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bt/w7zy5tlj00x2klxrqgm2w6dh0000gn/T/go-build150403232=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I was writing a lexer which unknowingly had a slicing memory error. It was working when it should not have been. I made a change to assign the tokens to a property on the lexer struct and it caused panics to appear causing the tests to fail.

I've created a reduced test case and it looks as if a decision comes down to inlining and when inlined the slicing panic does not occur as it should.

Inlined version that does not panic like it should:
https://play.golang.org/p/bdDhQWRj9GC

Not inlined version that does panic:
https://play.golang.org/p/r4KwyND4XdW

If you'd like the full context around the bug you can find the repo here:
https://github.com/pittfit/ortho/blob/5257d3027bda4989ff2e51edea082a95fa135263/lexer/lexer.go#L64

What did you expect to see?

A panic in both versions. Inlining should not affect program correctness.

What did you see instead?

A panic in the not inlined version only.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Nov 15, 2018

I think this is due to the allowed semantics of []byte("a"). It must return a slice of length 1, but the capacity might be larger. A [0:10] slice of the result may pass.
The choice of capacity may depend on other optimization decisions, like inlining.

If you replace []byte("a") with []byte("a")[:1:1] you get a panic as expected.

@randall77 randall77 closed this Nov 15, 2018
@thecrypticace

This comment has been minimized.

Copy link
Author

@thecrypticace thecrypticace commented Nov 15, 2018

Ah, you are correct. It appears that in the inlined version the capacity is always 32 (until your input string is larger) and in the not inlined version it's 8. Inlining decisions indeed affect the capacity of the byte slices. And it appears the increment of the capacity depends on inlining decisions as well.

This holds true even when converted from user input. So it's possible that a user could pass in a string and the failure of the program given various inputs could depend on whether or not the code was inlined. Would the recommended way to handle this be to slice the input and enforce the capacity to be equal to the length of the input to ensure you cannot read past the end of the buffer? (something like bytes = bytes[:len(bytes):len(bytes)]?)

Or should this be handled entirely by logic to ensure the read position isn't > len(bytes)? (I thought that's what I was doing but apparently that logic wasn't working right and the tests were not catching it because of the inlining)

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Nov 16, 2018

(something like bytes = bytes[:len(bytes):len(bytes)]?)

That's a good backstop. Your logic of course should make this not matter. It's only when the logic is wrong that this behavior is noticeable.
Proper unit tests should catch this kind of thing, as you're likely getting a wrong result if you're reading and using data beyond the end of the original string.

The other option is don't covert to []byte. Do everything you need using string.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.