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

runtime: writebarrierptr panic in isolated code snippet on Go 1.8.3 #21897

Closed
maxtaco opened this issue Sep 14, 2017 · 21 comments

Comments

Projects
None yet
7 participants
@maxtaco
Copy link

commented Sep 14, 2017

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

1.8.3.

Does this issue reproduce with the latest release?

The issue does not repro with 1.9

What operating system and processor architecture are you using (go env)?

Repro'ed on macOS and linux.

What did you do?

package main

import (
	"fmt"
	"unsafe"
	"time"
)

func main() {
	for i := 0; i < 1000000; i++ {
		x := unsafe.Pointer(uintptr(0x27))
		fmt.Printf("%p\n", x)
		time.Sleep(1)
	}
}

What did you expect to see?

No crash

What did you see instead?

....
0x27
0x27
0x27
0x27
0x27
0x27
runtime: writebarrierptr *0xc4200e80e0 = 0x27
fatal error: bad pointer in write barrier

runtime stack:
runtime.throw(0x10b6a19, 0x1c)
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/panic.go:596 +0x95
runtime.writebarrierptr.func1()
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/mbarrier.go:208 +0xbd
runtime.systemstack(0xc42001e600)
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/asm_amd64.s:327 +0x79
runtime.mstart()
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/proc.go:1132

goroutine 1 [running]:
runtime.systemstack_switch()
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/asm_amd64.s:281 fp=0xc420042cb8 sp=0xc420042cb0
runtime.writebarrierptr(0xc4200e80e0, 0x27)
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/mbarrier.go:209 +0x96 fp=0xc420042cf0 sp=0xc420042cb8
fmt.(*pp).printArg(0xc4200e80c0, 0x1097480, 0x27, 0x70)
	/usr/local/Cellar/go/1.8.3/libexec/src/fmt/print.go:605 +0xa10 fp=0xc420042d78 sp=0xc420042cf0
fmt.(*pp).doPrintf(0xc4200e80c0, 0x10b37d3, 0x3, 0xc420042f68, 0x1, 0x1)
	/usr/local/Cellar/go/1.8.3/libexec/src/fmt/print.go:998 +0x11e7 fp=0xc420042e58 sp=0xc420042d78
fmt.Fprintf(0x110a140, 0xc42000c018, 0x10b37d3, 0x3, 0xc420042f68, 0x1, 0x1, 0x13, 0x2, 0xc420446000)
	/usr/local/Cellar/go/1.8.3/libexec/src/fmt/print.go:181 +0x76 fp=0xc420042ec0 sp=0xc420042e58
fmt.Printf(0x10b37d3, 0x3, 0xc420042f68, 0x1, 0x1, 0x5, 0x0, 0x0)
	/usr/local/Cellar/go/1.8.3/libexec/src/fmt/print.go:190 +0x72 fp=0xc420042f20 sp=0xc420042ec0
main.main()
	/Users/max/src/go/src/play/play.go:12 +0x8c fp=0xc420042f88 sp=0xc420042f20
runtime.main()
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/proc.go:185 +0x20a fp=0xc420042fe0 sp=0xc420042f88
runtime.goexit()
	/usr/local/Cellar/go/1.8.3/libexec/src/runtime/asm_amd64.s:2197 +0x1 fp=0xc420042fe8 sp=0xc420042fe0
@maxtaco

This comment has been minimized.

Copy link
Author

commented Sep 14, 2017

Googling around I found #11689 and #15831, but both of those closed, I believe, without any resolution.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2017

This is expected behavior.
The garbage collector looks for invalid pointers when it is scanning objects. The pointer you constructed is considered invalid by the runtime, because it is non-nil and points to a place that can never be mapped.
We could conceivably allow these kind of pointers. However, we like this check because it helps ensure that the compiler+runtime is never getting the decision about what is a pointer and what isn't wrong (a source of many bugs historically).

@randall77 randall77 closed this Sep 15, 2017

@maxtaco

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

OK, thanks for taking a look.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2017

For valid uses of unsafe.Pointer, see https://golang.org/pkg/unsafe.

@akalin-keybase

This comment has been minimized.

Copy link

commented Sep 15, 2017

The original crash we're investigating was on macOS, and involved CFTypeRef from CoreFoundation. Here's a code snippet which shows a similar crash:

package main

/*
#cgo LDFLAGS: -framework CoreFoundation

#include <CoreFoundation/CoreFoundation.h>
*/
import "C"

import (
	"fmt"
	"time"
)

func main() {
	x := C.CFTypeRef(uintptr(0x27))
	for i := 0; i < 1000000; i++ {
		fmt.Printf("%p\n", x)
		time.Sleep(1)
	}
}

But why is the Go GC treating C.CFTypeRef like unsafe.Pointer, i.e. scanning it for invalid pointers?

@maxtaco

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

Or, I believe, this is equivalent:

/*

typedef const void *fooPtr;

*/
import "C"

import (
	"fmt"
	"time"
)

func main() {
	x := C.fooPtr(uintptr(0x27))
	for i := 0; i < 1000000; i++ {
		fmt.Printf("%p\n", x)
		time.Sleep(1)
	}
}

This comes up because some functions in the CoreFoundation library seems to be OK with these obviously non-pointer pointers, for instance CFGetTypeID.

@maxtaco

This comment has been minimized.

Copy link
Author

commented Sep 15, 2017

Slightly more digging, as I'm picking up a library that someone else wrote to solve this crasher, so a lot of this is new to me. As far as CoreFoundation is concerned, 0x27 is a CFNumber whose value is 0, which I think is meant to be interpreted as a nil pointer in some contexts. I also noted that a nil pointer doesn't trigger the write barrier check in mbarrier.go (for good reason), so this might be a case in which the GC is being overly aggressive in calling this a bad pointer. It's not a huge deal, we can workaround it in a number of ways. Just thought I'd report it in case it's of interest to the Go authors or if someone else hits the same problem.

Also of note, we found this pointer in an array with other valid pointers. That's the contract of the CoreFoundation library call we are using.

Also of note, we're never dereferencing this pointer, we're just copying it around and assigning it to other values, or printing it via %p in our little tester above. So we were surprised that the runtime presumed it pointed to valid memory.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2017

Yeah, it looks like Apple decided to pack non-pointer things into a pointer.

#include <CoreFoundation/CFNumber.h>
#include <stdio.h>

int main() {
	long long v[2] = {0x55555555555555, 0x555555555555555};
	for(int i = 0; i < 2; i++) {
		CFNumberRef n = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &v[i]);
		printf("%p\n", (void*)n);
	}
}

Prints

> gcc -framework CoreFoundation main.c
> ./a.out
0x5555555555555537
0x7f8912c06230

It looks like a CFNumberRef isn't always a real pointer. If the integer is at most 56 bits, it is encoded directly in the pointer and the low bits (?) determine the encoding.

This is unfortunate. I'm not sure what we can do about it. It bleeds up to CFTypeRef, the void* of the apple world.

Maybe we teach cgo that it should use uintptr for all these CF*Ref types? We can't reasonably allocate them on the Go side, so that might work. If cgo could reliably detect them...

It isn't just the pointer-to-the-zero-page problem noted earlier. We could manufacture a pointer to just about anywhere the runtime considers invalid (goroutine stacks, padding at the end of spans) using CFNumberCreate and just the right (or wrong, depending on your perspective) integer.

Reopening. I'm not sure we can fix this, but we should probably consider if there's anything sensible to do.

@randall77 randall77 reopened this Sep 15, 2017

@akalin-keybase

This comment has been minimized.

Copy link

commented Sep 15, 2017

Does that mean it's currently impossible to call e.g. C.CFNumberCreate from go safely, since it returns a C.CNumberRef? Or does immediately casting a C.C*Ref to a uintptr suffice?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2017

Casting to uintptr immediately should suffice.

@dominikh

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

OpenGL has a similar issue in glFenceSync, which returns a GLsync, which is an opaque struct pointer type (typedef struct __GLsync *GLsync;) that doesn't always represent actual pointers. See go-gl/gl#71 for discussions on that.

@akalin-keybase

This comment has been minimized.

Copy link

commented Sep 15, 2017

@dominikh, you mention in go-gl/gl#71 (comment) that doing e.g. C.SomeTaggedPointerType(unsafe.Pointer(x)) isn't safe in the context of a function call.

Isn't that pretty bad? In this context, I can call functions that return a C.CFTypeRef and immediately cast it to unsafe.Pointer and uintptr, but if I wanted to call a function that took a C.CFTypeRef, e.g. CFRelease to free the memory, then I'd have to cast it back to unsafe.Pointer, which then the GC can pick up and panic on.

Does that mean I need to write C shims to take uintptr_t for every function that currently takes a C.CFTypeRef that I want to call?

@dominikh

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

That is the conclusion I have reached, yes. I'd love to be proven wrong, though.

@akalin-keybase

This comment has been minimized.

Copy link

commented Sep 18, 2017

@randall77 @ianlancetaylor can either of you guys comment on whether casting a non-pointer value to unsafe.Pointer to pass to a C function taking void * is safe or unsafe? I'm hoping it's safe, since surely there are lots of C APIs that use void * as a generic pointer-sized type for parameters.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

@akalin-keybase That is unsafe at the moment.
There are two reasons:

  1. Garbage collection can happen at the start of the cgo wrapper for the C function.
  2. The go<->c pointer checker runs in this wrapper and would also see (and barf on? I'm not sure) this bad pointer.

The first we might be able to fix with the right sprinkling of nosplit directives in cgo generated code.
The second seems harder, as the test should fail on some pointers, and from C we can manufacture almost any pointer. Unless we turn this check off altogether.

Neither can happen on returns, so there immediately casting to uintptr is ok.

The only workaround I can see at the moment is to do the cast on the C side and pass uintptr across the c/go boundary.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

@akalin-keybase Go is not C, and Go has a simple rule: a variable of pointer type must hold a pointer value. Tricks like converting an integer to a pointer type, while valid in C, are not valid in Go.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 26, 2017

Change https://golang.org/cl/66332 mentions this issue: cmd/cgo: special case C ptr types to use uintptr

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

Tricks like converting an integer to a pointer type, while valid in C, are not valid in Go.

Note that even in C, “the result is implementation-defined, might not be correctly aligned, might not point to an entity of the referenced type, and might be a trap representation”, and in general, “[i]f the
resulting pointer is not correctly aligned for the referenced type, the behavior is undefined.”

So this isn't a portable idiom in C, either: it's only valid if you make non-standard assumptions about the C implementation.

@maxtaco

This comment has been minimized.

Copy link
Author

commented Nov 17, 2017

Thank you very much for this!

@gopherbot

This comment has been minimized.

Copy link

commented Dec 5, 2017

Change https://golang.org/cl/81876 mentions this issue: cmd/cgo: make JNI's jobject type map to uintptr in Go

gopherbot pushed a commit that referenced this issue Dec 8, 2017

cmd/cgo: make JNI's jobject type map to uintptr in Go
The jobject type is declared as a pointer, but some JVMs
(Dalvik, ART) store non-pointer values in them. In Go, we must
use uintptr instead of a real pointer for these types.

This is similar to the CoreFoundation types on Darwin which
were "fixed" in CL 66332.

Update #22906
Update #21897

RELNOTE=yes

Change-Id: I0d4c664501d89a696c2fb037c995503caabf8911
Reviewed-on: https://go-review.googlesource.com/81876
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Dec 8, 2017

Change https://golang.org/cl/82917 mentions this issue: doc: add doc about C types that we map to uintptr instead of ptr

gopherbot pushed a commit that referenced this issue Dec 8, 2017

doc: add doc about C types that we map to uintptr instead of ptr
Update #22906
Update #21897

Change-Id: I73709b2fdac6981d4bc2f7dab0767f2dd7be3be5
Reviewed-on: https://go-review.googlesource.com/82917
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Dec 8, 2018

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