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 Pointer-to-uintptr conversion for Syscall #23051

Closed
mdempsky opened this issue Dec 8, 2017 · 4 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Dec 8, 2017

Package unsafe's docs say:

The compiler handles a Pointer converted to a uintptr in the argument list of a call to a function implemented in assembly by arranging that the referenced allocated object, if any, is retained and not moved until the call completes, even though from the types alone it would appear that the object is no longer needed during the call.

That is, it handles an unsafe.Pointer converted to uintptr.

However, compiling this program with "go tool compile -live":

package p

import (
        "syscall"
        "unsafe"
)

func f() {
        p := new(byte)
        syscall.Syscall(syscall.SYS_READ, 0, uintptr(unsafe.Pointer(p)), 1)
}

func g() {
        p := unsafe.Pointer(new(byte))
        syscall.Syscall(syscall.SYS_READ, 0, uintptr(p), 1)
}

shows that only f ensures that the pointer is kept alive through the call to syscall.Syscall:

r.go:10:17: live at call to Syscall: .autotmp_1

This is because ordercall uses the IsPtr predicate, which only matches on normal Go pointers. It should actually use IsUnsafePtr.

/cc @ianlancetaylor

@mdempsky mdempsky changed the title cmd/compile: //go:uintptrescapes handles pointers and unsafe.Pointers differently cmd/compile: mishandling of Pointer-to-uintptr conversion for Syscall Dec 8, 2017
@gopherbot
Copy link

Change https://golang.org/cl/82817 mentions this issue: cmd/compile: fix unsafe.Pointer liveness for Syscall-like functions

@mdempsky
Copy link
Member Author

mdempsky commented Dec 8, 2017

Uploaded a fix for this. This issue has existed since this logic was introduced in CL 18584, so it's not a regression, but it is a potential memory corruption issue.

@mdempsky mdempsky added this to the Go1.10 milestone Dec 8, 2017
@mdempsky
Copy link
Member Author

mdempsky commented Dec 8, 2017

Marking as Go1.10 to make sure it gets a decision. I'm fine deferring to 1.11, but I'd expect we'd backport for 1.10.x in that case, so maybe it makes sense to just include for 1.10.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 8, 2017
@ianlancetaylor
Copy link
Contributor

I think this needs to be fixed in 1.10 and probably in 1.9 also.

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

3 participants