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

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

Closed
mdempsky opened this issue Mar 22, 2018 · 17 comments
Closed

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

mdempsky opened this issue Mar 22, 2018 · 17 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

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 The path to resolution is known, but the work has not been done. label Mar 22, 2018
@mdempsky mdempsky added this to the Go1.11 milestone Mar 22, 2018
@mdempsky
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member Author

@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
Copy link
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
Copy link
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.

@mdempsky
Copy link
Member Author

https://play.golang.org/p/KEf2cpdL_mA demonstrates the go/defer issue. (From #38581, which I closed as duplicate of this.)

@cuonglm
Copy link
Member

cuonglm commented Sep 5, 2020

https://play.golang.org/p/KEf2cpdL_mA demonstrates the go/defer issue. (From #38581, which I closed as duplicate of this.)

It's interesting that as of go1.15, the program now sometimes print ok for defer (just run it several times on the playground link).

@mdempsky
Copy link
Member Author

mdempsky commented Sep 6, 2020

@cuonglm Interesting observation. It looks like the "go" test occasionally fails too. It looks like calling runtime.GC() twice in a row makes the failure reliable: https://play.golang.org/p/a4Na2EBF7nu

Edit: Actually, it's still not reliable. I just ran it again on the playground, and I saw "1 / 100" for defer.

@gopherbot
Copy link

Change https://golang.org/cl/253457 mentions this issue: cmd/compile: fix mishandling of unsafe-uintptr arguments in go/defer

gopherbot pushed a commit that referenced this issue Sep 9, 2020
Currently, the statement:

	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.

This CL fixes the issue, by wrapping g call in a closure:

	go func(p unsafe.Pointer) {
		g(uintptr(p))
	}(f())

then this will be rewritten into:

	tmp := f()
	go func(p unsafe.Pointer) {
		g(uintptr(p))
		runtime.KeepAlive(p)
	}(tmp)
	runtime.KeepAlive(tmp)  // superfluous, but harmless

So the unsafe.Pointer p will be kept alive at the time g call runs.

Updates #24491

Change-Id: Ic10821251cbb1b0073daec92b82a866c6ebaf567
Reviewed-on: https://go-review.googlesource.com/c/go/+/253457
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@cuonglm
Copy link
Member

cuonglm commented Sep 11, 2020

@mdempsky I wonder how can we write a test for the second case (tmp is kept alive longer than expected?). The first can be test by:

package main

import (
	"runtime"
	"unsafe"
)

func setup() unsafe.Pointer {
	s := "ok"
	runtime.SetFinalizer(&s, func(p *string) { *p = "FAIL" })
	return unsafe.Pointer(&s)
}

//go:noinline
//go:uintptrescapes
func test(s string, p uintptr) int {
	runtime.GC()
	if *(*string)(unsafe.Pointer(p)) != "ok" {
		panic(s + " return unexpected result")
	}
	return 0
}

func f() int {
	return test("tmp is not actually keepalive", uintptr(setup()))
}

func main() {
	println(f())
}

@gopherbot
Copy link

Change https://golang.org/cl/254397 mentions this issue: cmd/compile: attach OVARLIVE nodes to OCALLxxx

@mdempsky
Copy link
Member Author

@cuonglm I think you just need an expression like:

before(uintptr(setup())) + after()

And then we want to make sure the value is still alive in before, but has been garbage collected in after.

For example, this appears to work:

package main

import (
	"runtime"
	"unsafe"
	"sync/atomic"
)

var done uint32

func setup() unsafe.Pointer {
	s := "ok"
	runtime.SetFinalizer(&s, func(p *string) { atomic.StoreUint32(&done, 1) })
	return unsafe.Pointer(&s)
}

//go:noinline
//go:uintptrescapes
func before(p uintptr) int {
	if atomic.LoadUint32(&done) != 0 {
		panic("GC early")
	}
	return 0
}

func after() int {
	if atomic.LoadUint32(&done) == 0 {
		panic("GC late")
	}
	return 0
}

func main() {
	_ = before(uintptr(setup())) + after()
}

@dr2chase
Copy link
Contributor

I worry this test could easily be flaky; finalizers are run in another thread, with no particular guarantee of timeliness.

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2020

I worry this test could easily be flaky; finalizers are run in another thread, with no particular guarantee of timeliness.

Yes, it’s flaky, but we end up with different one afb5fca

@mdempsky
Copy link
Member Author

@dr2chase Yeah, I don't think there's any good way at the moment to block until all pending, runnable finalizers have run. I've filed #41362 as what I think would be the most robust solution here, but I'm open to other suggestions.

@golang golang locked and limited conversation to collaborators Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

9 participants