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

Jump to bottom

runtime: several FreeBSD syscall wrappers seem to mishandle errors #43106

Closed
benesch opened this issue Dec 9, 2020 · 1 comment
Closed

runtime: several FreeBSD syscall wrappers seem to mishandle errors #43106

benesch opened this issue Dec 9, 2020 · 1 comment
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.16

Comments

@benesch
Copy link
Contributor

benesch commented Dec 9, 2020 •
edited

I'm in the process of porting gccgo to FreeBSD and noticed something sketchy about some of the FreeBSD-specific syscall wrappers in the runtime package. Consider sys_umtx_op for example:

go/src/runtime/sys_freebsd_amd64.s

Lines 13 to 22 in 89f465c

TEXT runtime·sys_umtx_op(SB),NOSPLIT,$0
MOVQ addr+0(FP), DI
MOVL mode+8(FP), SI
MOVL val+12(FP), DX
MOVQ uaddr1+16(FP), R10
MOVQ ut+24(FP), R8
MOVL $454, AX
SYSCALL
MOVL AX, ret+32(FP)
RET

The caller of this function expects negative values to be errors and positive values to be successes:

go/src/runtime/os_freebsd.go

Lines 168 to 171 in 904e113

ret := sys_umtx_op(addr, _UMTX_OP_WAIT_UINT_PRIVATE, val, unsafe.Sizeof(*utp), utp)
if ret >= 0 || ret == -_EINTR {
return
}

But sys_umtx_op returns the result of the syscall (the AX register) directly, and according to the FreeBSD calling convention, the carry flag indicates whether AX contains an error code or successful return code, not the sign of the result. Most of the other syscalls in the package seem to get this right (anything that contains a JCC instruction after SYSCALL, roughly speaking, looks right to me), but a few others seem broken in the same way, like thr_new and pipe2.

#10052 seems to be some prior art on this subject. As a result a number of these syscall wrappers were fixed to inspect the carry flag, but not all of them.

Clearly in practice this doesn't seem to matter much, but probably worth fixing nonetheless.
@ianlancetaylor ianlancetaylor added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 10, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.16 milestone Dec 10, 2020
@gopherbot
Copy link

Change https://golang.org/cl/276892 mentions this issue: runtime: correct errors handling in several FreeBSD syscall wrappers

@egonelbre egonelbre mentioned this issue Feb 14, 2021
Open
@golang golang locked and limited conversation to collaborators Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
3 participants
@benesch @ianlancetaylor @gopherbot