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

syscall: the wrong sysctl was gimped on iOS #35101

Closed
zx2c4 opened this issue Oct 23, 2019 · 9 comments
Closed

syscall: the wrong sysctl was gimped on iOS #35101

zx2c4 opened this issue Oct 23, 2019 · 9 comments
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone
Go1.14

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Oct 23, 2019

In issue #34133, @mmaxim reported that the app store was rejecting his app due to the use of __sysctl, which he blamed on the Go runtime. Others didn't seem to have the problem, but @mmaxim said that things were accepted by the app store if he went back to Go 1.10. @eliasnaur considered this to be confirmation of the problem and removed sysctl on iOS from Go 1.14 and backported that to Go 1.13.3.

Unfortunately the diagnosis was incorrect. __sysctl is undocumented, but sysctl is documented. The Go runtime uses sysctl. However, x/sys/unix uses __sysctl. @mmaxim noticed that Go 1.10 worked for him, because going back that far switched his vendored x/sys/unix over to using raw syscalls instead of going through libSystem.

This is a regression, breaking wireguard-apple's use of net.InterfaceByName, which uses sysctl under the hood.
@zx2c4
Copy link
Contributor Author

zx2c4 commented Oct 23, 2019

CC @crawshaw

@gopherbot
Copy link

Change https://golang.org/cl/202778 mentions this issue: syscall: reenable sysctl on iOS

@gopherbot
Copy link

Change https://golang.org/cl/202837 mentions this issue: unix: __sysctl is sysctl on darwin

@zx2c4
Copy link
Contributor Author

zx2c4 commented Oct 23, 2019

@gopherbot Please backport this to 1.13, because it fixes a grave regression in the net package introduced in 1.13.3.

@gopherbot gopherbot mentioned this issue Oct 23, 2019
Closed
@gopherbot
Copy link

Backport issue(s) opened: #35105 (for 1.13).

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

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 23, 2019
@dmitshur dmitshur added this to the Go1.14 milestone Oct 23, 2019
@dmitshur dmitshur added mobile Android, iOS, and x/mobile OS-Darwin labels Oct 23, 2019
@zx2c4 zx2c4 mentioned this issue Oct 23, 2019
Closed
gopherbot pushed a commit to golang/sys that referenced this issue Oct 23, 2019
@zx2c4
unix: __sysctl is sysctl on darwin
f89234f 
While the other BSDs use __sysctl as the name, Darwin now uses sysctl,
without the leading underscores, and considers __sysctl to be "private".
Using __sysctl leads to App Store rejections, and Go's syscall package
already uses the proper syscall. So this commit changes Darwin's syscall
to use it too here, while reverting a recent commit that removed it all
together on arm and arm64.

This reverts CL 194097.

Fixes golang/go#35103
Updates golang/go#34133
Updates golang/go#35101

Change-Id: Ic72d5e7a435b99fe62c533b77b2c3790590f4c9e
Reviewed-on: https://go-review.googlesource.com/c/sys/+/202837
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@gopherbot
Copy link

Change https://golang.org/cl/202779 mentions this issue: [release-branch.go1.13] syscall: reenable sysctl on iOS

@aaronbee
Copy link

aaronbee commented Oct 24, 2019 •
edited

This change broke macOS 10.14 at least.

package main

import (
	"golang.org/x/sys/unix"
)

func main() {
	unix.Sysctl("kern.boottime")
}

Compiling this program results in:

golang.org/x/sys/unix.libc_sysctl_trampoline·f: relocation target golang.org/x/sys/unix.libc_sysctl_trampoline not defined

@aaronbee
Copy link

The darwin assembly files still refer to __sysctl

zsyscall_darwin_386.s
43:TEXT ·libc___sysctl_trampoline(SB),NOSPLIT,$0-0
44:	JMP	libc___sysctl(SB)

zsyscall_darwin_amd64.s
43:TEXT ·libc___sysctl_trampoline(SB),NOSPLIT,$0-0
44:	JMP	libc___sysctl(SB)

zsyscall_darwin_arm64.s
43:TEXT ·libc___sysctl_trampoline(SB),NOSPLIT,$0-0
44:	JMP	libc___sysctl(SB)
``

@gopherbot
Copy link

Change https://golang.org/cl/202958 mentions this issue: unix: regenerate darwin libc trampolines after CL 202837

gopherbot pushed a commit to golang/sys that referenced this issue Oct 24, 2019
@tklauser
unix: regenerate darwin libc trampolines after CL 202837
e66fe6e 
CL 202837 forgot to properly re-generate
zsyscall_darwin_{386,amd64,arm64}.s with the correct trampoline name.

Updates golang/go#35103
Updates golang/go#34133
Updates golang/go#35101

Change-Id: I98805988f97c7ff51da858fdc36c436aa680c8c7
Reviewed-on: https://go-review.googlesource.com/c/sys/+/202958
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jason A. Donenfeld <Jason@zx2c4.com>
gopherbot pushed a commit that referenced this issue Oct 25, 2019
@zx2c4 @ianlancetaylor
[release-branch.go1.13] syscall: reenable sysctl on iOS
7ad8def 
This was disabled due to a report that the App Store rejects the symbol
__sysctl. However, we use the sysctl symbol, which is fine. The __sysctl
symbol is used by x/sys/unix, which needs fixing instead. So, this
commit reenables sysctl on iOS, so that things like net.InterfaceByName
can work again.

This reverts CL 193843, CL 193844, CL 193845, and CL 193846.

Fixes #35105
Updates #35101
Updates #34133
Updates #35103

Change-Id: Ib8eb9f87b81db24965b0de29d99eb52887c7c60a
Reviewed-on: https://go-review.googlesource.com/c/go/+/202778
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/202779
Reviewed-by: Elias Naur <mail@eliasnaur.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
4 participants
@zx2c4 @aaronbee @dmitshur @gopherbot