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

cmd/compile: unsafe.Add bug when adding uint8 value to a pointer #48536

Closed
rip-create-your-account opened this issue Sep 22, 2021 · 9 comments
Closed
Assignees
Milestone

Comments

@rip-create-your-account

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

go version go1.17.1 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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/anon/.cache/go-build"
GOENV="/home/anon/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/anon/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/anon/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build726424511=/tmp/go-build -gno-record-gcc-switches"

What did you do?

See https://play.golang.org/p/KR_dZZU0BDN. I used unsafe.Add in a loop to randomly advance a unsafe.Pointer that points to a byte array. The len argument type is uint8 where value is randomly 0 or 1. Also there's a function that would manipulate the value behind the pointer but it's not used.

What did you expect to see?

I expected the program to exit cleanly as the unsafe operations seem valid.

What did you see instead?

SIGSEGV crash for line 22 on the second iteration of the loop because the first dstNext = unsafe.Add(dstNext, uint8(adv)) seems to store a garbage value to dstNext. When len is of any other integer type the program exits cleanly. Try changing the line 23 to dstNext = unsafe.Add(dstNext, uint16(adv)). Also removing the unused function makes the program exit cleanly.

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 22, 2021

This looks like a bug with unsafe.Add of smaller-than-word offsets.
The generated code is:

	0x0068 00104 (/Users/khr/gowork/issue48536.go:19)	SETHI	CL
	0x006b 00107 (/Users/khr/gowork/issue48536.go:23)	ADDQ	"".dstNext+64(SP), CX
	0x0070 00112 (/Users/khr/gowork/issue48536.go:23)	MOVQ	CX, "".dstNext+64(SP)

But SETHI only writes the low byte of CX. The upper portions are junk, which in this case happen to be some other pointer. We end up adding two pointers together, which is not a valid pointer.

@mdempsky

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Sep 22, 2021

I think we just need to convert the len to uintptr.

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Sep 22, 2021

@randall77 darwin/arm64 does not fail, the generated code:

	0x0078 00120 (p.go:19)	CSET	HI, R4
	0x007c 00124 (p.go:23)	ADD	R4, R2, R2
	0x0080 00128 (p.go:23)	MOVD	R2, "".dstNext-8(SP)

any idea why?

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 22, 2021

x86 has weird legacy semantics for instructions like SETHI CL, where they only partially overwrite the CX register, leaving the rest of the bytes alone.

I'd assume that just doesn't apply to arm64 and instructions there always overwrite the full register.

Loading

@ianlancetaylor ianlancetaylor changed the title unsafe.Add bug when adding uint8 value to a pointer cmd/compile: unsafe.Add bug when adding uint8 value to a pointer Sep 22, 2021
@ianlancetaylor ianlancetaylor added this to the Go1.18 milestone Sep 22, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 22, 2021

@gopherbot Please open a backport issue to Go 1.17.

This appears to be a bug in the implementation of unsafe.Add. This should be fixed to avoid silent miscompilation of valid code.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2021

Backport issue(s) opened: #48561 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Loading

@mdempsky mdempsky self-assigned this Sep 22, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Sep 22, 2021

Simplified test case:

package main

import "unsafe"

var i = 257

func main() {
	var buf [10]byte
	p0 := unsafe.Pointer(&buf[0])
	p1 := unsafe.Pointer(&buf[1])

	if p := unsafe.Add(p0, uint8(i)); p != p1 {
		println("FAIL:", p, "!=", p1)
	}

	var x uint8
	if i != 0 {
		x = 1
	}
	if p := unsafe.Add(p0, x); p != p1 {
		println("FAIL:", p, "!=", p1)
	}
}

Loading

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Sep 22, 2021

@cuonglm @mdempsky yes, ARM64 CSET instruction writes the full register.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2021

Change https://golang.org/cl/351592 mentions this issue: cmd/compile: fix unsafe.Add with small-size offsets operands

Loading

@gopherbot gopherbot closed this in 9345323 Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants