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.

Sign up for GitHub

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

x/mobile: apps built with go 1.13, still rejected by Apple app store #34133

Closed
mmaxim opened this issue Sep 6, 2019 · 22 comments
Closed

x/mobile: apps built with go 1.13, still rejected by Apple app store #34133

mmaxim opened this issue Sep 6, 2019 · 22 comments
Labels
FrozenDueToAge mobile Android, iOS, and x/mobile
Milestone
Unreleased

Comments

@mmaxim
Copy link

mmaxim commented Sep 6, 2019

What version of Go are you using (go version)?

$ go version
1.13

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

gomobile for iOS.

What did you do?

Build an app and try to submit to Apple App Store.

What did you expect to see?

Acceptance.

What did you see instead?

The fix provided here #31628 is not complete. ptrace is not the only symbol that is being rejected by the store. Go >= 1.12 (including the latest release with a fix for the aforementioned ptrace problem) still triggers a rejection on the sysctl symbol. Some sample text from Apple:

ITMS-90338: Non-public API usage - The app references non-public symbols in Keybase: ___sysctl

The only way we can submit our app to the app store is to run the now unsupported Go 1.10.8, which works.
@gopherbot gopherbot added this to the Unreleased milestone Sep 6, 2019
@gopherbot gopherbot added the mobile Android, iOS, and x/mobile label Sep 6, 2019
@hyangah
Copy link
Contributor

hyangah commented Sep 6, 2019

@khr @eliasnaur

Can we disable the use of sysctl as well in the syscall package?

@eliasnaur
Copy link
Contributor

Sure, should we backport the fix as well?

@eliasnaur
Copy link
Contributor

@mmaxim, can you provide exact reproduction steps? I ask because my scatter.im app went into the App Store (testflight) just fine.

@mmaxim
Copy link
Author

mmaxim commented Sep 6, 2019

Unfortunately, I do not have a small repro for this (although I haven't really tried to get one). Right now this just affects when we submit the Keybase (https://github.com/keybase/client) app (which is substantial). We are likely doing something in there that is causing this symbol to get included, but I don't know what it would be. Like I said, it works for us with 1.10.8, but we have been stuck there because of this problem.

Note that before 1.13 we saw ptrace in the error from Apple as well, but that is now gone, so that previous fix seemed effective for us on that symbol.

@mmaxim
Copy link
Author

mmaxim commented Sep 6, 2019

Also, if you get a patch together at some point, I'd be happy to test it out on Keybase if that would help.

@eliasnaur
Copy link
Contributor

What I would like with a reproducer is to avoid the whack-a-mole game of disabling one symbol at a time. Perhaps I can include all syscalls in a synthetic reproducer.

@mmaxim
Copy link
Author

mmaxim commented Sep 6, 2019

Yeah that totally makes sense. For what it's worth the error from Apple lists all the offending symbols in our submission. Before 1.13, that list was ptrace and sysctl. On 1.13, that list is now down to sysctl.

@eliasnaur
Copy link
Contributor

@mmaxim please test https://golang.org/cl/193843.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193843 mentions this issue: syscall: disable sysctl on iOS

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/194097 mentions this issue: unix: disable sysctl on iOS

@eliasnaur
Copy link
Contributor

@gopherbot please consider this for backport to 1.13 to allow Go apps through the App Store checks.

@gopherbot gopherbot mentioned this issue Sep 7, 2019
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #34170 (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.

gopherbot pushed a commit to golang/sys that referenced this issue Sep 7, 2019
@eliasnaur
unix: disable sysctl on iOS
d223b2b 
Running the regenerating scripts also brought in ClockGettime.

Updates golang/go#34133

Change-Id: I0eb9ed6dbbc2bdd7e3d2a7f5d88492e9dfed0ada
Reviewed-on: https://go-review.googlesource.com/c/sys/+/194097
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193844 mentions this issue: log/syscall: skip unsupported tests on iOS

gopherbot pushed a commit that referenced this issue Sep 7, 2019
@eliasnaur
log/syslog: skip unsupported tests on iOS
a5025fd 
CL 193843 disabled sysctl on iOS. This change disables two tests that
rely on sysctl.

Updates #34133

Change-Id: I7c569a1992a50ad6027a294c1fd535cccddcfc4e
Reviewed-on: https://go-review.googlesource.com/c/go/+/193844
Run-TryBot: Elias Naur <mail@eliasnaur.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193845 mentions this issue: net,os: disable more sysctl tests on iOS

gopherbot pushed a commit that referenced this issue Sep 7, 2019
@eliasnaur
net,os: disable more sysctl tests on iOS
2711fab 
Updates #34133

Change-Id: I27c75993176cf876f2d80f70982528258c509b68
Reviewed-on: https://go-review.googlesource.com/c/go/+/193845
Run-TryBot: Elias Naur <mail@eliasnaur.com>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/193846 mentions this issue: syscall: re-generate zsyscall_darwin_arm*.so

gopherbot pushed a commit that referenced this issue Sep 8, 2019
@eliasnaur
syscall: re-generate zsyscall_darwin_arm*.s
83a78eb 
I missed that in CL 193843.

Updates #34133

Change-Id: I70b420f022cc7f8289f07375bfc2ade20cf3ffe7
Reviewed-on: https://go-review.googlesource.com/c/go/+/193846
Run-TryBot: Elias Naur <mail@eliasnaur.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
@mmaxim
Copy link
Author

mmaxim commented Sep 10, 2019

@eliasnaur sorry for the delay, just got to this today. It worked! Thanks!

@zx2c4
Copy link
Contributor

zx2c4 commented Oct 23, 2019 •
edited
Loading

ITMS-90338: Non-public API usage - The app references non-public symbols in Keybase: ___sysctl

Are you sure that ___sysctl is from Go and not from some other library you're pulling in?

I've been shipping Go 1.13-generated binaries in the iOS app store that use sysctl without a problem.

This commit, which was backported to 1.13, wound up breaking net.InterfaceByName (operation unsupported, since it uses sysctl under the hood), which means it broke WireGuard.

CC @crawshaw

@zx2c4
Copy link
Contributor

zx2c4 commented Oct 23, 2019

I just cloned @mmaxim's keybase client repo, and apparently the issue is actually that it uses a vendored copy of x/sys/unix which uses the non-public symbol __sysctl. This is a different symbol than the public symbol sysctl. On Go 1.10, which @mmaxim says he was using before, x/sys/unix makes the raw syscall. Beyond 1.10, it uses a jump into libSystem, but improperly.

The issue here seems to be, therefore, with x/sys/unix, not with Go. Go 1.14 and 1.13.3 therefore regress. I'll submit a patch fixing the regression, and @eliasnaur - you can adjust x/sys/unix if you want.

This was referenced Oct 23, 2019
Closed
Closed
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Oct 23, 2019
@zx2c4
syscall: reenable sysctl on iOS
f91e895 
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 #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>
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
Contributor

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

@gopherbot
Copy link
Contributor

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
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@zx2c4 @eliasnaur @mmaxim @hyangah @gopherbot