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.

Sign up for GitHub

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

x/sys/windows: clarify safety of Proc.Call, LazyProc.Call #42680

Open
bradfitz opened this issue Nov 17, 2020 · 5 comments
Open

x/sys/windows: clarify safety of Proc.Call, LazyProc.Call #42680

bradfitz opened this issue Nov 17, 2020 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

https://godoc.org/golang.org/x/sys/windows#LazyProc.Call and https://godoc.org/golang.org/x/sys/windows#Proc.Call are syscall.SysCall wrappers but take a bunch of uintptrs.

I assume that those wrappers are not covered by the uintptr(unsafe.Pointer) conversion exception number (4) from https://golang.org/pkg/unsafe/#Pointer, correct?

Could we document that either way in x/sys/windows?

I assume that many callers will require runtime.KeepAlive?

/cc @ianlancetaylor @mdempsky @alexbrainman @josharian @crawshaw
@bradfitz bradfitz added the Documentation Issues describing a change to documentation. label Nov 17, 2020
@gopherbot gopherbot added this to the Unreleased milestone Nov 17, 2020
@mdempsky
Copy link
Contributor

I think this is Q1 of #34684.

@mdempsky
Copy link
Contributor

Also, runtime.KeepAlive would only guarantee that the actual pointer is still live. It doesn't guarantee that uintptr conversion stays valid. Calling Proc.Call might require growing the stack, which could invalidate a pointer to stack memory.

@bradfitz bradfitz mentioned this issue Nov 18, 2020
Closed
@bradfitz
Copy link
Contributor Author

bradfitz commented Nov 18, 2020 •
edited
Loading

Yeah, this could be closed as a dup of #34684 I suppose.

But I'm not exactly sure the status of that bug. It sure sounds like {Lazy,}Proc.Call just aren't safe to use today, at least for arguments involving pointers.

Should we clarify that in the docs for now while we wait for something to happen with #34684?

@mdempsky
Copy link
Contributor

Direct calls to {Lazy,}Proc.Call should be fine, at least since b3bd7ab (first included in Go 1.14).

Either way, I'm happy to have the docs clarified.

@bradfitz
Copy link
Contributor Author

Ah! I had just missed the //go:uintptrescapes lines in that file. Okay, that makes me feel much better.

I'll send some docs, thanks!

@bradfitz bradfitz self-assigned this Nov 20, 2020
@petemoore petemoore mentioned this issue Jan 28, 2022
Closed
petemoore added a commit to taskcluster/taskcluster that referenced this issue Feb 3, 2022
@petemoore
generic-worker no longer relies on the undocumented go pragma //go:ui…
ee51f2d 
…ntptrescapes

The unofficial documentation for this pragma can be found in the source code:
  * https://github.com/golang/go/blob/go1.17.6/src/cmd/compile/internal/noder/lex.go#L71-L81

Instead of relying on this pragma, generic-worker now calls
syscall.Syscall<x> directly. This change has been made since I suspect
that nested go:uintptrescapes functions may not work as expected, but
furthmore, since this pragma is not an official feature, it presumably
may change over time, so we probably should not assume it will always be
supported.

There have also been several issues raised against functions and methods
that rely on its behaviour, so it is not clear whether it is entirely
safe to use:

  * golang/go#16035
  * golang/go#23045
  * golang/go#34474
  * golang/go#34642
  * golang/go#34684
  * golang/go#34810
  * golang/go#42680

Note, in future an option might be to generate the function bodies using mkwinsyscall:
  * https://github.com/golang/go/blob/go1.17.6/src/internal/syscall/windows/mksyscall.go#L9
  * https://github.com/golang/go/blob/go1.17.6/src/syscall/mksyscall_windows.go#L47
  * https://pkg.go.dev/golang.org/x/sys/windows/mkwinsyscall
  * https://github.com/golang/sys/blob/master/windows/mkwinsyscall/mkwinsyscall.go

One advantage of this approach is that we can probably log the syscalls
again using:

  * https://github.com/golang/sys/blob/50617c2ba19781ae46f34bb505064996b8fa32e8/windows/mkwinsyscall/mkwinsyscall.go#L43-L44

Alternatively if tracing doesn't do what we expect, we could write our own generator.
@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 23, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@mknyszek mknyszek moved this to Triage Backlog in Go Compiler / Runtime Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @mdempsky @cagedmantis @gopherbot