runtime: checkptr incorrectly -race flagging when using &^ arithmetic

[SnapDragon64]

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

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

go run -race the following code:

package main

import (
	"fmt"
	"unsafe"
)

func main() {
	a := make([]int, 100)
	b := unsafe.Pointer(&a)
	c := unsafe.Pointer(uintptr(b) + 12)
	fmt.Println(b, c)
	d := unsafe.Pointer(uintptr(c) - (uintptr(c) & 7))
	fmt.Println(d)
	e := unsafe.Pointer(uintptr(c) &^ 7)
	fmt.Println(e)
}

What did you expect to see?

All pointers stay within bounds, so the program should work.

What did you see instead?

0xc00000e060 0xc00000e06c
0xc00000e068
fatal error: checkptr: pointer arithmetic result points to invalid allocation

goroutine 1 [running]:
runtime.throw(0x116a2c2, 0x40)
	/usr/local/Cellar/go/1.15/libexec/src/runtime/panic.go:1116 +0x72 fp=0xc000105ea0 sp=0xc000105e70 pc=0x1072e72
runtime.checkptrArithmetic(0xc00000e068, 0x0, 0x0, 0x0)
	/usr/local/Cellar/go/1.15/libexec/src/runtime/checkptr.go:43 +0xbe fp=0xc000105ed0 sp=0xc000105ea0 pc=0x1046bde
main.main()
	/Users/dkisman/go/src/foo/foo.go:15 +0x26f fp=0xc000105f88 sp=0xc000105ed0 pc=0x1136daf
runtime.main()
	/usr/local/Cellar/go/1.15/libexec/src/runtime/proc.go:204 +0x209 fp=0xc000105fe0 sp=0xc000105f88 pc=0x1075649
runtime.goexit()
	/usr/local/Cellar/go/1.15/libexec/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000105fe8 sp=0xc000105fe0 pc=0x10a5781
exit status 2

Note that the d pointer arithmetic works fine, and e flags the error, despite the two being logically equivalent.

[randall77]

@mdempsky Looks like it's passing a zero-length originals array to checkptrArithmetic. Probably need to expand how we track down originals to handle & and &^ (| also?).

[mdempsky]

Thanks for the report. That test case is correct, so it's definitely an issue that checkptr is reporting a false positive about unsafe.Pointer(uintptr(c) &^ 7).

There's logic in the checkptr instrumentation to try to recognize &^ here:

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

Line 4006 in 46ca7b5

case OSUB, OANDNOT:

But it looks like this is probably failing because we've already recursively walked the subexpression and rewritten it into unsafe.Pointer(uintptr(c) & (^7)) here:

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

Lines 973 to 976 in 46ca7b5

	case OANDNOT:
	n.Left = walkexpr(n.Left, init)
	n.Op = OAND
	n.Right = nod(OBITNOT, n.Right, nil)

Probably the easiest fix is to look for OAND+OBITNOT instead of OANDNOT. But maybe we can run the instrumentation before recursively walking instead.

--

@randall77

Probably need to expand how we track down originals to handle & and &^ (| also?).

https://golang.org/pkg/unsafe/#Pointer says:

It is valid both to add and to subtract offsets from a pointer in this way. It is also valid to use &^ to round pointers, usually for alignment.

So I don't think we need to handle & or |, and cmd/vet doesn't like them either: https://github.com/golang/tools/blob/b793a1359eac7d446e5c21c6c5ce58496109fb50/go/analysis/passes/unsafeptr/unsafeptr.go#L115

[mdempsky]

Of note, unsafe.Pointer(uintptr(p) & ^uintptr(7)) is invalid, but the checkptr error message ("pointer arithmetic result points to invalid allocation") will probably be confusing to people who haven't read the fine print about unsafe.Pointer arithmetic. E.g., as @SnapDragon64 points out, it does stay within bounds, so it's not really an "invalid allocation" per se.

Probably easiest is to just tweak the error message.

[mdempsky]

@SnapDragon64 Can you share how you ran into this issue? In particular, do you have real world code that was already using &^ and is broken by upgrading to Go 1.15? Or were you just experimenting with pointer arithmetic? I'm trying to weigh whether the fix should be backported to 1.15.

[gopherbot]

Change https://golang.org/cl/249477 mentions this issue: cmd/compile: fix checkptr handling of &^

[mdempsky]

@dmitshur Do you mind weighing in on whether this merits backporting to Go 1.15?

Arguments for:

• It's a Go compiler regression that causes valid Go code to fail at runtime.
• It was introduced in Go 1.14, and as Go 1.13 just went out of support, users now have to upgrade to Go 1.14 / Go 1.15.
• The fix is quite small and self-contained (CL 249477).

Arguments against:

• It's an obscure code pattern (pointer arithmetic using &^), as evidenced by the bug laying undiscovered for at least 6 months.
• It only affects -race / -msan builds, and it fails in a very loud and actionable way identifying the culprit line of code (albeit with a potentially confusing error message).
• As a workaround, users can rewrite p &^ x to p - (p & x).

[SnapDragon64]

@SnapDragon64 Can you share how you ran into this issue? In particular, do you have real world code that was already using &^ and is broken by upgrading to Go 1.15? Or were you just experimenting with pointer arithmetic? I'm trying to weigh whether the fix should be backported to 1.15.

Yes, we had some real world code using &^ , and when we upgraded to 1.15 our tests started failing. (I don't think it would affect production, which doesn't use -race.) However, since there's an easy workaround (converting to a subtraction), it wasn't too difficult to fix.

[mdempsky]

@gopherbot Please consider for backport to Go 1.15. See my rationale for/against at #40917 (comment). Also, that this affected real user code at #40917 (comment).

[gopherbot]

Backport issue(s) opened: #40934 (for 1.15).

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

[dmitshur]

@mdempsky Thank you for providing useful information for the backporting decision. We've discussed the 1.15 cherry-pick candidate in a release meeting.

Per our release policy (see #34536 (comment)), since this bug is also present in Go 1.14, we must either backport to both 1.15+1.14 or neither. Do you think this fix is safe to backport to 1.14 as well?

I've tried it locally and saw that it applied cleanly, other than a merge conflict in TestCheckPtr test cases.

[mdempsky]

@dmitshur Thanks for pointing that policy out to me. Yes, I think it should be safe to backport to 1.14 as well. I just looked at the diff between 1.14 and 1.15 for cmd/compile, and nothing significant changed about either checkptr or the internal Implicit flag that the fix uses.

Edit: The conflict is just because the we tweaked the error messages in 1.15 to be clearer. None of the underlying logic for when to report errors changed.

[gopherbot]

Change https://golang.org/cl/249879 mentions this issue: [release-branch.go1.15] cmd/compile: fix checkptr handling of &^

[gopherbot]

Change https://golang.org/cl/249838 mentions this issue: [release-branch.go1.14] cmd/compile: fix checkptr handling of &^

[mdempsky]

@SnapDragon64 Thanks for the issue report. A fix should be included in Go 1.15.1, once it comes out, and then your workaround shouldn't be necessary any further. Please let us know if you run into any further false positives.