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: way to check if pointer satisfies (*Pinner).Pin #62356

Closed
gucio321 opened this issue Aug 29, 2023 · 10 comments
Closed

runtime: way to check if pointer satisfies (*Pinner).Pin #62356

gucio321 opened this issue Aug 29, 2023 · 10 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@gucio321
Copy link
Contributor

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

$ go version

go version go1.21.0 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/username/.cache/go-build'
GOENV='/home/username/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/username/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/username/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3016021898=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I'm writing a C wrapper library. I'm trying to handle void* pointer. I'm experiencing crashes about cgo pointer has Go pointer to Go pointer, so I've updated to the latest go release (1.21) and now panic adives to use Pinner.
When I use Pinner it works sometimes, but sometimes it crashes because of panic: runtime error: runtime.Pinner.Pin: argument is not a Go pointer.
I've used dlv to chack this pointer and it seems to be unsafe.Pointer as usual, so I have no idea why it panics this time.

Sadly I have no reproducable code snippet. The only thing I can post is link to my dev branch in project when this happens: https://github.com/gucio321/cimgui-go/tree/input-text-with-hint

What did you expect to see?

I think that even if this isn't a bug in Pinner, there should be some way to check if a pointer (or unsafe.Pointer) is a "Go Pointer"

What did you see instead?

I see no way to fix my bug

I'm sory if I've used a wrong template

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 29, 2023
@gucio321 gucio321 changed the title runtime: Pinner panics for unknown reason: way to check if pointer is a "Go pointer" runtime: way to check if pointer satisfies (*Pinner).Pin Aug 29, 2023
@ianlancetaylor
Copy link
Contributor

A "Go pointer" is a pointer to memory that was allocated by Go code in the ordinary way, rather than by using something like syscall.Mmap. To be clear, it has nothing whatsoever to do with the type of the pointer.

In general it should be possible to keep track of where you allocate your pointers. Passing a C pointer (a pointer to memory allocated by C code) to runtime.Pinner.Pin is a logic error.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 29, 2023
@gucio321
Copy link
Contributor Author

In general it should be possible to keep track of where you allocate your pointers. Passing a C pointer (a pointer to memory allocated by C code) to runtime.Pinner.Pin is a logic error.

Sure, but it is quite difficult to determine if specific unsafe.Pointer was allocated by go or c. It should be possible but I'd duplicate logic from runtime package and it'll ofc hit performance of my code. I saw a function runtime.isGoPointerXXXX. is it possible to have it exported?

@rsc
Copy link
Contributor

rsc commented Aug 30, 2023

It does seem like there is a problem here. If you are writing a general-purpose library that wraps C code, then you may want to pin the pointers being passed into your library before passing them onward to C. Pure Go general-purpose libraries can be invoked with memory that was allocated by mmap or C (provided they don't retain those pointers after returning), but this kind of C wrapper using pinner cannot.

Perhaps Pin should silently allow pinning of non-Go pointers (there is nothing for it to do) instead of panicking.

@gucio321
Copy link
Contributor Author

Perhaps Pin should silently allow pinning of non-Go pointers (there is nothing for it to do) instead of panicking.

It'd fix my issue perfectly. However I'm wondering if it doesn't violate compatibility 😕

e.g. code like this
ptr := C.get_my_ptr()
p := &runtime.Pinner{}
defer func() {
   if r := recover() {
      fmt.Println("Pointer is not go pointer")
   }
}()
p.Pin(ptr)
//...

@ianlancetaylor
Copy link
Contributor

I, at least, am not worried about that level of compatibility.

doujiang24 added a commit to doujiang24/go that referenced this issue Sep 9, 2023
People may don't know the detail of a pointer, this make the
runtime.Pinner.Pin API easier to use.

Fixes: golang#62356
doujiang24 added a commit to doujiang24/go that referenced this issue Sep 9, 2023
People may don't know the detail of a pointer, this make the
runtime.Pinner.Pin API easier to use.

Fixes golang#62356
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/527156 mentions this issue: runtime: Pin silently allow pinning of non-Go pointers

@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 10, 2023
@heschi heschi added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 11, 2023
@heschi heschi modified the milestones: Backlog, Go1.22 Sep 11, 2023
@heschi
Copy link
Contributor

heschi commented Sep 11, 2023

cc @golang/runtime

@mknyszek
Copy link
Contributor

I agree it's super annoying that Pin panics. The original intent was to mirror SetFinalizer, but I agree it's completely reasonable to do something different here. Thanks @doujiang24 for sending a CL!

@mknyszek
Copy link
Contributor

Looking back at the reviews for the original Pinner implementation, it looks like the possibility came up but we preferred to stick close to SetFinalizer's semantics, I guess just as a conservative starting point.

One thing I'm slightly concerned about is that there's no explicit escape of pointers passed to Pin. One danger with ignoring non-heap pointers is that we could pretend to pin a stack pointer and then the stack could move. (We currently have no way to pin a stack pointer.) I'll comment on the CL.

@mknyszek
Copy link
Contributor

One thing I'm slightly concerned about is that there's no explicit escape of pointers passed to Pin. One danger with ignoring non-heap pointers is that we could pretend to pin a stack pointer and then the stack could move. (We currently have no way to pin a stack pointer.) I'll comment on the CL.

Just kidding. I forgot that we put the pointer into (*pinner).refs and the *pinner has a finalizer. So it's always on the heap, and in turn anything passed to Pin has to escape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants