runtime, x/sys/unix: Connectx is broken on darwin/amd64

[database64128]

Go version

go version go1.23.5 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/database64128/Library/Caches/go-build'
GOENV='/Users/database64128/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/database64128/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/database64128/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.23.5/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.23.5/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.5'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/database64128/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/xd/v25clzgj08d6_cbbp7lwszfr0000gn/T/go-build880517508=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

@wwqgtxx reports that x/sys/unix.Connectx is broken on darwin/amd64. The call succeeds, but does not return the correct number of bytes sent, and may cause memory corruption (fatal error: stack not a power of 2).

They were able to work around the issue by making the following minimal change:

diff --git a/sys/unix/zsyscall_darwin_amd64.go b/sys/unix/zsyscall_darwin_amd64.go
index 24b346e..f8cc117 100644
--- a/sys/unix/zsyscall_darwin_amd64.go
+++ b/sys/unix/zsyscall_darwin_amd64.go
@@ -848,7 +848,7 @@ func connectx(fd int, endpoints *SaEndpoints, associd SaeAssocID, flags uint32,
 	} else {
 		_p0 = unsafe.Pointer(&_zero)
 	}
-	_, _, e1 := syscall_syscall9(libc_connectx_trampoline_addr, uintptr(fd), uintptr(unsafe.Pointer(endpoints)), uintptr(associd), uintptr(flags), uintptr(_p0), uintptr(len(iov)), uintptr(unsafe.Pointer(n)), uintptr(unsafe.Pointer(connid)), 0)
+	_, _, e1 := Syscall9(SYS_CONNECTX, uintptr(fd), uintptr(unsafe.Pointer(endpoints)), uintptr(associd), uintptr(flags), uintptr(_p0), uintptr(len(iov)), uintptr(unsafe.Pointer(n)), uintptr(unsafe.Pointer(connid)), 0)
 	if e1 != 0 {
 		err = errnoErr(e1)
 	}

Connectx was added by me in golang/sys@59665e5 (CL 606155). It does not have any issues on darwin/arm64. We spent hours on this and were not able to pinpoint the exact cause.

What did you see happen?

Connectx is broken on darwin/amd64.

What did you expect to see?

Connectx works on darwin/amd64.

[gabyhelp]

Related Issues

• cmd/link: Link bug on darwin arm64 #53495 (closed)
• x/sys: gomobile build failed for x/crypto/ssh/terminal #22727 (closed)
• cmd/link: Incorrect symbol linked in darwin/arm64 #58935 (closed)
• x/sys/unix: go:linkname must refer to declared function or variable #51091 (closed)
• cmd/go/internal/list: segmentation fault on darwin #67113 (closed)
• x/sys: some trampolines on darwin/amd64 still causing "unexpected return pc for runtime.sigpanic" #45939 (closed)
• x/sys/unix: add TCP_CONNECTION_INFO #53330 (closed)
• os/user: Lookup fails to find users when running as root on darwin-arm64 cross compiled from darwin-amd64 #43543
• clear #57553 (closed)
• Update #70550 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[mknyszek]

CC @golang/runtime

[mknyszek]

Switching to the underlying syscall instead of libc suggests we're probably holding Darwin's libc wrong? Michael P notes maybe stack corruption from a mistake in how it's called? macOS requires us to call through libc for everything, so I'm not sure we should switch to making the underlying syscall directly.

[randall77]

I think syscall9 is incorrect. The return values might not be plumbed back correctly. Try this patch:

diff --git a/src/runtime/sys_darwin.go b/src/runtime/sys_darwin.go
index 5c769a71ea..af10c53659 100644
--- a/src/runtime/sys_darwin.go
+++ b/src/runtime/sys_darwin.go
@@ -72,10 +72,11 @@ func syscall6()
 //go:nosplit
 //go:cgo_unsafe_args
 func syscall_syscall9(fn, a1, a2, a3, a4, a5, a6, a7, a8, a9 uintptr) (r1, r2, err uintptr) {
+       args := struct{ fn, a1, a2, a3, a4, a5, a6, a7, a8, a9, r1, r2, err uintptr }{fn, a1, a2, a3, a4, a5, a6, a7, a8, a9, 0, 0, 0}
        entersyscall()
-       libcCall(unsafe.Pointer(abi.FuncPCABI0(syscall9)), unsafe.Pointer(&fn))
+       libcCall(unsafe.Pointer(abi.FuncPCABI0(syscall9)), unsafe.Pointer(&args))
        exitsyscall()
-       return
+       return args.r1, args.r2, args.err
 }
 func syscall9()

[wwqgtxx]

I tested the patch provided by @randall77 and the result was still the same. It seems that the problem may not be caused by incorrect parsing of the return value.

[ianlancetaylor]

@randall77 I don't think there is a problem with syscall_syscall9. I think the problem you are pointing out is addressed by the use of //go:cgo_unsafe_args. I don't know why syscall_syscall6 doesn't use //go:cgo_unsafe_args, but I do see that syscall_syscall9 was added later (in https://go.dev/cl/446178).

What I wonder about instead is the assembler function runtime·syscall9 in runtime/sys_darwin_amd64.s. That function assumes that if we have 9 parameters, the last three will be passed in R10, R11, R12. But here we are using the calling convention for a C function, not the convention for a SYSCALL instruction. At least on Linux, when calling a C function with 9 parameters, the last three are not passed in R10, R11, R12; they are passed on the stack. For example, see page 20 of https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf.

As far as I know macOS is the same, though perhaps somebody could confirm that.

If that is the case then we need to change runtime·syscall9 to pass the last three arguments on the stack. We'll have to assume that they are all integer-shaped, which is likely for the cases where this is used.

Currently I believe that the only function that uses runtime·syscall9 other than Connectx is Getnameinfo in the internal/syscall/unix package. And the only argument that may be being passed incorrectly is the flags argument. That function is only called to do a reverse DNS name lookup. The function will mostly do the right thing regardless of the flag value. It may be that nobody has noticed any problem here.

[database64128]

I don't know why syscall_syscall6 doesn't use //go:cgo_unsafe_args, but I do see that syscall_syscall9 was added later (in https://go.dev/cl/446178).

The use of //go:cgo_unsafe_args was removed in 57e3809 (CL 386719). CL 446178 was opened months after CL 386719 was merged, but somehow didn't pick up the changes.

[database64128]

If that is the case then we need to change runtime·syscall9 to pass the last three arguments on the stack. We'll have to assume that they are all integer-shaped, which is likely for the cases where this is used.

Any chance we can get a fix for this? Thanks.

[gopherbot]

Change https://go.dev/cl/665435 mentions this issue: runtime: fix 9-arg syscall on darwin/amd64

[randall77]

Possible fixed listed above. Could someone try it?

[wwqgtxx]

I tested https://go.googlesource.com/go/+/87973aae0be01e1be75ccfa60dc4aa1e539400c8 on golang1.24.2 and found that it still cannot be used normally. Moreover, it is different from the previous data error and panic. The current phenomenon seems that Connectx returns success but is not actually executed.

[randall77]

I found a bug in my CL and pushed a fix. Could you try that?
(The bug was only on the error return path, so maybe there's something else wrong...)

[database64128]

I found a bug in my CL and pushed a fix. Could you try that? (The bug was only on the error return path, so maybe there's something else wrong...)

Thank you for the quick fix! I think this is it. In the tests @wwqgtxx ran, connectx(2) has a high chance of returning -EINPROGRESS, which was not properly propagated back to the caller with the previous patch. This explains the -ENOTCONN error returned by subsequent writes.

Still, let's wait until @wwqgtxx has the new results, just to be sure.

[wwqgtxx]

I found a bug in my CL and pushed a fix. Could you try that? (The bug was only on the error return path, so maybe there's something else wrong...)

Thanks for your help. After applying https://go.googlesource.com/go/+/a15345edabef8f40ae46e4e8117bd830ec0fb059, Connectx behaves normally and all tfo-go tests can pass on darwin amd64 platform.

[randall77]

Ok, that's good. I'll get that CL in soon.

I think the remaining question is, should we backport?
In favor:

1. This could cause really weird behavior and/or corruption on darwin/amd64. There is only one stdlib call affected (syscall/Getnameinfo), but several in x.sys (Connectx for freebsd, mmap for netbsd, dragonfly).
2. darwin/amd64 is a first-class port.

In opposition:

1. This has always been broken (since introduced in late 2022). It is not a regression from 1.24.
2. Only the Connectx failure has been noticed.

I'm leaning towards backporting. The fix is pretty simple and the unfixed behavior is very hard to debug and/or work around.

@gopherbot please open backport issues

[gopherbot]

Backport issue(s) opened: #73379 (for 1.24).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[database64128]

Should we also open a CL based on #71302 (comment), not for this issue, but like 57e3809 (CL 386719) did for #51247?

[randall77]

I was incorrect on that referenced comment.

Maybe we should fix syscall9 like we did in CL 386719, but I can't make it fail at the moment. This code

package main

import "net"

func main() {
	_, _ = net.LookupAddr("8.8.8.8")
}

Compiles just fine with -race and -gcflags=-N -l on darwin/arm64. So maybe that bug is not triggerable with just this.

It would be good to be consistent between the different syscall functions, though.

@cherrymui

[cherrymui]

Yeah, perhaps this particular code path doesn't use as much stack space. It may be still good to be consistent. If we apply the patch as #71302 (comment) , we should also remove the cgo_unsafe_arg pragma.

[gopherbot]

Change https://go.dev/cl/665695 mentions this issue: runtime: don't use cgo_unsafe_args for syscall9 wrapper