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: checkptr incorrectly -race flagging when using &^ arithmetic #40917

Closed
SnapDragon64 opened this issue Aug 20, 2020 · 14 comments
Closed

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

SnapDragon64 opened this issue Aug 20, 2020 · 14 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.16

Comments

@SnapDragon64
Copy link

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

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

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 mdempsky self-assigned this Aug 20, 2020
@mdempsky
Copy link
Member

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

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

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

@mdempsky
Copy link
Member

mdempsky commented Aug 20, 2020 •
edited

@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).

@ALTree ALTree added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 20, 2020
@SnapDragon64
Copy link
Author

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

@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 gopherbot mentioned this issue Aug 20, 2020
Closed
@gopherbot
Copy link

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

@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 mdempsky mentioned this issue Aug 21, 2020
Closed
@mdempsky
Copy link
Member

mdempsky commented Aug 21, 2020 •
edited

@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.

@dmitshur dmitshur mentioned this issue Aug 21, 2020
Closed
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Aug 21, 2020
@mdempsky @dmitshur
[release-branch.go1.15] cmd/compile: fix checkptr handling of &^
f454f70 
checkptr has code to recognize &^ expressions, but it didn't take into
account that "p &^ x" gets rewritten to "p & ^x" during walk, which
resulted in false positive diagnostics.

This CL changes walkexpr to mark OANDNOT expressions with Implicit
when they're rewritten to OAND, so that walkCheckPtrArithmetic can
still recognize them later.

It would be slightly more idiomatic to instead mark the OBITNOT
expression as Implicit (as it's a compiler-generated Node), but the
OBITNOT expression might get constant folded. It's not worth the extra
complexity/subtlety of relying on n.Right.Orig, so we set Implicit on
the OAND node instead.

To atone for this transgression, I add documentation for nodeImplicit.

Updates #40917.
Fixes #40934.

Change-Id: I386304171ad299c530e151e5924f179e9a5fd5b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/249477
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
(cherry picked from commit e94544c)
Reviewed-on: https://go-review.googlesource.com/c/go/+/249879
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
gopherbot pushed a commit that referenced this issue Aug 21, 2020
@mdempsky @dmitshur
[release-branch.go1.14] cmd/compile: fix checkptr handling of &^
24dbf3e 
checkptr has code to recognize &^ expressions, but it didn't take into
account that "p &^ x" gets rewritten to "p & ^x" during walk, which
resulted in false positive diagnostics.

This CL changes walkexpr to mark OANDNOT expressions with Implicit
when they're rewritten to OAND, so that walkCheckPtrArithmetic can
still recognize them later.

It would be slightly more idiomatic to instead mark the OBITNOT
expression as Implicit (as it's a compiler-generated Node), but the
OBITNOT expression might get constant folded. It's not worth the extra
complexity/subtlety of relying on n.Right.Orig, so we set Implicit on
the OAND node instead.

To atone for this transgression, I add documentation for nodeImplicit.

Updates #40917.
Fixes #40968.

Change-Id: I386304171ad299c530e151e5924f179e9a5fd5b8
Reviewed-on: https://go-review.googlesource.com/c/go/+/249477
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/249838
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@mdempsky
Copy link
Member

@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.

@dmitshur dmitshur added this to the Go1.16 milestone Aug 22, 2020
@golang golang locked and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
6 participants
@mdempsky @dmitshur @SnapDragon64 @ALTree @randall77 @gopherbot