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

cmd/compile: mishandling of unsafe-uintptr arguments #24491

Open
mdempsky opened this Issue Mar 22, 2018 · 7 comments

Comments

Projects
None yet
5 participants
@mdempsky
Member

mdempsky commented Mar 22, 2018

When calling certain functions that take uintptr parameters with unsafe.Pointer arguments, the compiler rewrites the call to ensure the pointer is kept alive through the call. (See #13372 for more background.) For example, given

func f() unsafe.Pointer
func g(uintptr) int

the expression statement

g(uintptr(f()))

gets rewritten during order into (roughly)

tmp := f()
g(uintptr(tmp))
runtime.KeepAlive(tmp)    // actually OVARLIVE, but same idea

There are a handful of related issues with how this is currently implemented. (I expect some of these will be solved separately, so splitting them into separate issues may be appropriate. I'm just noting them all together for now in case it helps us think of a more holistic solution.)

  1. If the caller directly returns the results, the KeepAlive statement is ineffectual because it's dropped by ssa.go as dead code. That's because

    return g(uintptr(f()))
    

    gets rewritten into

    tmp := f()
    return g(uintptr(tmp))
    runtime.KeepAlive(tmp)
    

    so tmp isn't actually kept alive through the call to g.

  2. Because the KeepAlives are added after the statement, the pointer is kept alive longer than necessary. For example,

    h(g(uintptr(f()))
    

    gets rewritten into

    tmp := f()
    h(g(uintptr(tmp)))
    runtime.KeepAlive(tmp)
    

    so tmp is kept alive

  3. They don't work with defer or go calls. For example,

    go g(uintptr(f()))
    

    gets rewritten into

    tmp := f()
    newproc(8, g, uintptr(tmp))
    runtime.KeepAlive(tmp)
    

    which doesn't guarantee that tmp is still alive by time the g call is scheduled to run.

I expect 1 and 2 can be fixed by attaching the KeepAlives directly to the OCALLFUNC node, so that we can insert the OpVarLive immediately after the OpStaticCall/etc.

I expect 3 will need to be fixed by wrapping the calls in a closure.

/cc @ianlancetaylor @aclements

@mdempsky mdempsky added the NeedsFix label Mar 22, 2018

@mdempsky mdempsky added this to the Go1.11 milestone Mar 22, 2018

@mdempsky

This comment has been minimized.

Member

mdempsky commented Mar 22, 2018

Also for context, these most likely only affect syscall.Syscall* and Window's syscall.(*Proc).Call and syscall.(*LazyProc).Call, which probably aren't commonly (if ever) used in these ways. So the risk of this being a problem in practice is probably pretty low.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Mar 22, 2018

Apparently, we do use defer syscall.Syscall in some runtime tests:

https://github.com/golang/go/blob/master/src/runtime/memmove_linux_amd64_test.go#L46
https://github.com/golang/go/blob/master/src/runtime/race/race_windows_test.go#L33

but neither includes a uintptr conversion, so they're safe.

And (*Proc).Call uses return Syscall, but also without uintptr conversions:

https://github.com/golang/go/blob/master/src/syscall/dll_windows.go#L144

I couldn't find any obvious at-risk calls within Google's Go code base.

@josharian

This comment has been minimized.

Contributor

josharian commented Mar 22, 2018

Mostly unrelated, but: It would be nice if defer runtime.KeepAlive(x) was as efficient as inserting a keep-alive of x on every exit path. This would avoid having to manually rewrite return f(x) into tmp := f(x); return tmp in cgo wrapper code. I mention it only because the underlying implementation mechanism might be similar. Apologies for the hijack.

@aclements

This comment has been minimized.

Member

aclements commented Mar 23, 2018

Is there anything preventing us from recognizing this pattern when we lower call ASTs to SSA and inserting the keep-alives then? I think this is similar to your suggestion, but I'm proposing to take this out of "order" entirely.

Case 3 is interesting. I think you're right that the only option may be to wrap it in a closure, since (at least for go), nothing we do in the calling function can keep the pointer live long enough.

@mdempsky

This comment has been minimized.

Member

mdempsky commented Mar 23, 2018

@josharian That sounds like an interesting issue, but seems pretty different in scope from this one to me. I suspect it overlaps more significantly with the ideas we've had to optimize exactly-once defer statements.

@aclements We might be able to move all of the work into (*state).call. The only issue I can see with that is walk.go will have already rewritten the argument list into explicit OINDREGSP assignments, so it just might be a little clumsy piecing things back together.

@dr2chase

This comment has been minimized.

Contributor

dr2chase commented Jun 26, 2018

See also: https://go-review.googlesource.com/c/go/+/114797
Moving call construction into ssa ought to make this easier.
I'd like to revisit if/when we get that CL in, fixes should be much cleaner.

@dr2chase dr2chase modified the milestones: Go1.11, Go1.12 Jun 26, 2018

@odeke-em

This comment has been minimized.

Member

odeke-em commented Dec 9, 2018

I'd like to revisit if/when we get that CL in, fixes should be much cleaner.

Hello @dr2chase, CL https://go-review.googlesource.com/c/go/+/114797 was merged in October 2018, I am just paging you to let you know as per your comment above. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment