Dynamic unsafe pointer checking available at master

[Matthew Dempsky]

I just committed CL 162237, which adds a -d=checkptr compiler flag to cmd/compile to enable dynamic instrumentation for checking unsafe pointer arithmetic and conversions. See the commit message for details about what exactly it currently detects.

To enable it, simply build/test with -gcflags=all=-d=checkptr.

For example, here it found some incorrect pointer arithmetic in syscall.UnixRights (namely, that this code is written in a C style using "just past the end" pointers, which are not allowed in Go; fix at CL 201617):

$ go test -gcflags=all=-d=checkptr -run=Rights syscall
--- FAIL: TestUnixRightsRoundtrip (0.00s)
panic: (runtime.ptrArith) (0x561b40,0xc00000e0c0) [recovered]
	panic: (runtime.ptrArith) (0x561b40,0xc00000e0c0)

goroutine 7 [running]:
testing.tRunner.func1(0xc000108100)
	/usr/local/google/home/mdempsky/wd/go/src/testing/testing.go:874 +0x3a3
panic(0x561b40, 0xc00000e0c0)
	/usr/local/google/home/mdempsky/wd/go/src/runtime/panic.go:679 +0x1b2
syscall.cmsgData(...)
	/usr/local/google/home/mdempsky/wd/go/src/syscall/sockcmsg_unix.go:54
syscall.UnixRights(0xc000111b98, 0x0, 0x0, 0x3, 0x0, 0x0)
	/usr/local/google/home/mdempsky/wd/go/src/syscall/sockcmsg_unix.go:97 +0xfc
syscall_test.TestUnixRightsRoundtrip(0xc000108100)
	/usr/local/google/home/mdempsky/wd/go/src/syscall/syscall_unix_test.go:298 +0x69d
testing.tRunner(0xc000108100, 0x591618)
	/usr/local/google/home/mdempsky/wd/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
	/usr/local/google/home/mdempsky/wd/go/src/testing/testing.go:960 +0x351
FAIL	syscall	0.026s
FAIL

[Emmanuel Odeke]

Nice work, Matthew and thank you!

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAF52%2BS7a6AmT0E5qc1Wj2RrNtQqzkzNVn1LQv6M4NeKbT9db_g%40mail.gmail.com.

[gugl.z...@gmail.com]

Great stuff!  Enabling on all personal projects!

[Bryan C. Mills]

Nice!

Would it be feasible to enable this in a dashboard builder? (Want to file an issue?)

--

[Matthew Dempsky]

On Thu, Oct 17, 2019, 5:40 AM Bryan C. Mills <bcm...@google.com> wrote:

Would it be feasible to enable this in a dashboard builder? (Want to file an issue?)

Yeah, I think it should be reasonable. The instrumentation only affects conversions involving unsafe.Pointer, so it should be pretty lightweight. It's also compatible with -race and -msan, so we could probably just enable it on any existing instrumented builders.

[gugl.z...@gmail.com]

It doesn't seem to catch conversions from T1 to T2 where T2 is larger.
E.g. https://play.golang.org/p/32lWMkFPvm5.  Are these not supported
yet?  Should I file an issue?

[Brad Fitzpatrick]

What about just making -race enable it? (for all users, not just the dashboard)

How expensive is it compared to race's existing overhead?

 

--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAF52%2BS4L%2BS9TmNv9DrronASYY6iwnYAyesBHGg4hnVFx4iJNEg%40mail.gmail.com.

[Matthew Dempsky]

Detecting that is not currently supported, but I think there may at least be some cases where we can detect it. Please go ahead and file an issue.

[Matthew Dempsky]

On Thu, Oct 17, 2019, 10:22 AM Brad Fitzpatrick <brad...@golang.org> wrote:

What about just making -race enable it? (for all users, not just the dashboard)

Yeah, that seems reasonable too.

How expensive is it compared to race's existing overhead?

I would expect it's less expensive, but I haven't measured this at all.

[gugl.z...@gmail.com]

> Detecting that is not currently supported, but I think there may at
> least be some cases where we can detect it. Please go ahead and file
> an issue.

Filed https://github.com/golang/go/issues/34959.  Thanks again for the
tool, looking forward to using it in 1.14!

[Asif Jalil]

Is the checkptr option compatible with cgo? I have the following code:

 

       //use ODBC v3
        ret = C.SQLSetEnvAttr(C.SQLHENV(drv.henv),
                C.SQL_ATTR_ODBC_VERSION,
                C.SQLPOINTER(uintptr(C.SQL_OV_ODBC3)), 0)

 
checkptr option gives me this error: 

./driver.go:147:61: cannot use _Ctype_PTR value as type unsafe.Pointer in argument to runtime.checkptrArithmetic

[Matthew Dempsky]

On Thu, Oct 17, 2019 at 12:52 PM Asif Jalil <asif.m...@gmail.com> wrote:

Is the checkptr option compatible with cgo?

It should be, but as you point out, currently it's not. I've filed https://github.com/golang/go/issues/34966, and should have it fixed shortly.

[Mark Rushakoff]

Matthew,

Great work here. Thanks for getting this in.

Would it be possible to improve the panic messages? I would expect that running with checkptr will become general advice, but "runtime.ptrArith" and "runtime.ptrAlign" aren't as instructive as they probably could be. I don't have experience fixing these errors yet to offer a meaningful suggestion on wording improvement, but even the wording in the commit message is much more clear than the current panic messages. 

--

You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAF52%2BS7a6AmT0E5qc1Wj2RrNtQqzkzNVn1LQv6M4NeKbT9db_g%40mail.gmail.com.

[Matthew Dempsky]

On Fri, Oct 18, 2019 at 10:28 PM Mark Rushakoff <ma...@influxdata.com> wrote:

Great work here. Thanks for getting this in.

Thanks!

Would it be possible to improve the panic messages?

Yeah, the current messages are definitely bad. The instrumentation code is at: https://github.com/golang/go/blob/master/src/runtime/checkptr.go

The ptrAlign and ptrArith types just need to be turned into proper error types (e.g., with "Error() string" methods), and panic will print them appropriately.

The only annoying part is that package runtime can't use fmt or strconv, so the strings need to be manually constructed. There's some code to do this in boundsError.Error that could maybe be refactored.

[Matthew Dempsky]

As an update, unsafe pointer checking is now enabled automatically when using -race or -msan (except on Windows). 

Also, thanks to everyone who's tested it out and reported issues or helped fix up failures in the standard library!

[Lorenz Bauer]

Hi Matthew,

Thanks for working on this! I tried this on a piece of code where I've been wondering whether it's a valid use of unsafe.Pointer:

https://github.com/cilium/ebpf/blob/f79c574/perf/ring.go#L55

Using gotip and -race I get no warnings. Does this mean it's valid? In that case, can we amend the text in unsafe to cover this more specifically? If not, can we warn about it, and is there a correct way to achieve this?

Lorenz

[Keith Randall]

On Wed, Oct 23, 2019 at 6:13 AM 'Lorenz Bauer' via golang-dev <golan...@googlegroups.com> wrote:

Hi Matthew,

Thanks for working on this! I tried this on a piece of code where I've been wondering whether it's a valid use of unsafe.Pointer:

https://github.com/cilium/ebpf/blob/f79c574/perf/ring.go#L55

Using gotip and -race I get no warnings. Does this mean it's valid? In that case, can we amend the text in unsafe to cover this more specifically? If not, can we warn about it, and is there a correct way to achieve this?

The checkptr tests are not guaranteed to find all violations - a lack of a report does not mean code is valid.

Your code is technically invalid. It fits under case (1), but is technically invalid because byte and PerfEventMmapPage do not "share an equivalent memory layout".

In this case it should be fine, though. mmap should return a sufficiently sized and aligned region (your code would fail if you used &mmap[1] or &mmap[len(mmap)-1] instead of &mmap[0] due to alignment or size issues),

and because PerfEventMmapPage contains no pointers it shouldn't make the GC unhappy.

I think the checkptr improvements double-check the alignment constraints, so it would catch &mmap[1]. It does not have the smarts to make sure size is big enough.

 

Lorenz

On Wednesday, October 23, 2019 at 1:16:35 AM UTC+1, Matthew Dempsky wrote:

As an update, unsafe pointer checking is now enabled automatically when using -race or -msan (except on Windows). 

Also, thanks to everyone who's tested it out and reported issues or helped fix up failures in the standard library!

--

You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/405385f3-5e08-4d61-810b-e4c95d780155%40googlegroups.com.