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

Comments

Projects
None yet
3 participants
@mdempsky
Copy link
Member

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

This comment has been minimized.

Copy link

commented Dec 8, 2017

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2017

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

@gopherbot gopherbot closed this in 840fad1 Dec 8, 2017

@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.
You can’t perform that action at this time.