Closed brunnre8 closed 1 year ago
This is safe.
v
will be kept alive through ioctl
call.ioctl
does uintptr(arg)
where arg
is an uintptr
, not a pointer.cc @mdempsky
I don't think that code is safe.
The ioctl
function does not have any pragmas to mark it as a syscall function, so the compiler won't know to do anything special for it. So the call to ioctl
could potentially grow the stack, and then the arg
pointer would be stale pointing to old memory.
I don't think that code is safe.
The
ioctl
function does not have any pragmas to mark it as a syscall function, so the compiler won't know to do anything special for it. So the call toioctl
could potentially grow the stack, and then thearg
pointer would be stale pointing to old memory.
Hmm, so, whether it violates unsafe pointer rule?
Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr 🤔
Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr 🤔
or using go:nosplit
Maybe the right fix is passing in an unsafe.Pointer instead of an uintptr
I think so. The Linux ioctl
man page at least says that the third argument is always a pointer. (If there are any ioctl
s that do actually pass a scalar value directly instead of a pointer, they'll need a separate utility wrapper.)
@mdlayher added IoctlSetInt
for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.
@mdlayher added
IoctlSetInt
for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.
From https://man7.org/linux/man-pages/man2/ioctl.2.html:
The third argument is an untyped pointer to memory
From https://docs.oracle.com/cd/E23824_01/html/821-1463/ioctl-2.html
The arg argument represents a third argument that has additional information that is needed by this specific device to perform the requested function. The data type of arg depends upon the particular control request, but it is either an int or a pointer to a device-specific data structure.
It thus seems like IoctlSetInt
was made for solaris
consistent with the description in #20474. It should never be used for linux
.
Relevant issue: https://github.com/golang/go/issues/34684
Change https://golang.org/cl/340915 mentions this issue: unix: generate ioctlNew on Linux with unsafe.Pointer arg
@mdempsky, @ianlancetaylor, @mdlayher: was this fixed by CL 340915, or is there more to do here?
I believe the reason I didn't close this before was because I only fixed it for Linux. Similar changes would have to be made for *BSD and etc.
@mdlayher added
IoctlSetInt
for Linux in https://golang.org/cl/44009 for #20474. That suggests that Linux has some ioctls that don't take a pointer value for the third argument, although off hand I do not know what they are.
Here we are, FICLONE
https://man7.org/linux/man-pages/man2/ioctl_ficlonerange.2.html ... which I didn't knew existed a few moments ago 😁
Change https://go.dev/cl/469315 mentions this issue: unix: add ioctlPtr with unsafe.Pointer arg on other unices
Change https://go.dev/cl/469675 mentions this issue: syscall: avoid passing Go pointers as uintptr in exec tests
Change https://go.dev/cl/469835 mentions this issue: unix: add ptracePtr that accepts pointer arg as unsafe.Pointer
Change https://go.dev/cl/471119 mentions this issue: unix: add ioctlPtr with unsafe.Pointer arg on other unices (cont)
@dmgk, can this issue be closed now, or is there more work remaining for it?
I believe this is done, closing.
Thanks a lot to everyone involved
From the documentation of unsafe.Pointer:
Now, as far as I can tell this forces packages to adhere to exactly that.
The code in x/sys clearly violates that rule.
in unix/ioctl.go there's
and the declaration of ioctl in zsyskall_linux.go:
I've asked on the mailing list if that's valid usage of unsafe, @ianlancetaylor wasn't sure and told me to open an issue here for further discussion. (https://groups.google.com/g/golang-nuts/c/Ocpy8CKs7ZI/m/3ym8gnuBFgAJ)