-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Use futex-based synchronization on Apple platforms #122408
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
base: master
Are you sure you want to change the base?
Conversation
|
||
// These syscalls appeared with macOS 10.12. | ||
weak! { | ||
pub fn __ulock_wait(u32, *mut c_void, u64, u32) -> c_int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind explaining more what the thought process of these going undetected is? Seems like a risky chance that might require a hot fix if the MAS or iOS app store scanners see through this.
In reality I think this won't be enough and we need to call ulock
things via syscall directly instead. This means that the "fallback" Apple futex implementation is only going to be usable on macOS.
I know Electron apps on the MAS use direct syscalls to emulate private functions, so this is probably what Rust should do too on macOS with precedence:
#define SYS_ulock_wait 515
#define SYS_ulock_wake 516
#define SYS_ulock_wait2 544
For iOS, detect if the os_sync_
functions are available or fallback to the non-futex implementation :/ Calling a syscall
directly is forbidden per the iOS headers so std
can't do the same as macOS:
__WATCHOS_PROHIBITED __TVOS_PROHIBITED
__OS_AVAILABILITY_MSG(ios,deprecated=10.0,"syscall(2) is unsupported; "
"please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost().")
__OS_AVAILABILITY_MSG(macosx,deprecated=10.12,"syscall(2) is unsupported; "
"please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost().")
int syscall(int, ...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a "spirit of the law" kind of situation. Apple understandably wants App Store software to continue working in newer versions, so they prohibit private API use, as they want to be able to remove or change that API at will. By only using the private API as fallback and not linking it directly, we fulfil this wish. If we directly linked the private API instead, we'd get dynamic linker errors if they remove it, so we can't do that.
On the other hand, we break the "letter of the law" as we are using private API. In my opinion, this is totally fine and justified, but of course, this is only my interpretation. It would be great if someone from Apple could look over this, just so that we know that they are aware of this. My past attempt at reaching out to them for this has been unsuccessful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider the solution of atomic-wait
? They use libc++.
https://github.com/m-ou-se/atomic-wait/blob/main/src/macos.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these only exist starting with macOS 11, which is way above our minimum of 10.12, so the ulock fallback would be needed regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets precedent for using private APIs in the standard library (our current use of weak!
is only used for symbols that are available in newer versions), which I think is a fairly big deal, and should perhaps be discussed more broadly (in a separate issue? (that could be FCP'ed?)).
Somewhat related is #114186. Tagging in particular @thomcc and @workingjubilee, as they seem to have been involved in this kind of stuff before?
At the very least, we should use the dlsym!
macro explicitly here, to make it very clear that we're not weakly linking the symbol (which would be very visible in the binary), but actually loading it at runtime, and thereby evading the App Store's checks (dlsym
itself seems to be allowed).
I believe this is sufficient such that we do not need to do raw syscalls (which I remember to have caused problems for Go in the past since as the ABI isn't stable (?)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we shouldn't be calling this directly, it'll trip the app store static analysis.
wait(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, value, 0); | ||
} | ||
} else { | ||
panic!("your system is below the minimum supported version of Rust"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These panics also get to be deleted if raw syscalls are used :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do keep the panics, I'd go for an abort here instead, this is not recoverable in any way and avoiding the overhead from unwind landing pads would be nice.
☔ The latest upstream changes (presumably #122423) made this pull request unmergeable. Please resolve the merge conflicts. |
Implement the `os_unfair_lock` functions on macOS These are needed for rust-lang/rust#122408. See the documentation [here](https://developer.apple.com/documentation/os/synchronization?language=objc) and the implementation [here](https://github.com/apple-oss-distributions/libplatform/blob/a00a4cc36da2110578bcf3b8eeeeb93dcc7f4e11/src/os/lock.c#L645).
Implement the `os_unfair_lock` functions on macOS These are needed for rust-lang#122408. See the documentation [here](https://developer.apple.com/documentation/os/synchronization?language=objc) and the implementation [here](https://github.com/apple-oss-distributions/libplatform/blob/a00a4cc36da2110578bcf3b8eeeeb93dcc7f4e11/src/os/lock.c#L645).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, nice work, this will be a great perf improvement!
I know it is a draft, but once you're ready, it'd be nice to split the Mutex
changes apart from the futex changes, if possible.
@rustbot label O-apple
target_os = "macos", | ||
target_os = "ios", | ||
target_os = "tvos", | ||
target_os = "watchos", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use target_vendor = "apple"
, as that also includes visionOS. Also applies elsewhere.
/// See os/os_sync_wait_on_address.h (Apple has failed to upload the documentation | ||
/// to their website) for documentation of the public API and | ||
/// https://github.com/apple-oss-distributions/xnu/blob/1031c584a5e37aff177559b9f69dbd3c8c3fd30a/bsd/sys/ulock.h#L69 | ||
/// for the header file of the private API, along with its usage in libpthread | ||
/// https://github.com/apple-oss-distributions/libpthread/blob/d8c4e3c212553d3e0f5d76bb7d45a8acd61302dc/src/pthread_cond.c#L463 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also link the design/decision document:
https://github.com/apple-oss-distributions/libplatform/blob/libplatform-316.100.10/docs/expose-futex-style-api.md
if let Some(wake) = os_sync_wake_by_address_any.get() { | ||
unsafe { wake(addr, size_of::<u32>(), OS_SYNC_WAKE_BY_ADDRESS_NONE) == 0 } | ||
} else if let Some(wake) = __ulock_wake.get() { | ||
// __ulock_wake can get interrupted, so retry until either waking up a | ||
// waiter or failing because there are no waiters (ENOENT). | ||
loop { | ||
let r = unsafe { wake(UL_COMPARE_AND_WAIT | ULF_NO_ERRNO, addr, 0) }; | ||
|
||
if r >= 0 { | ||
return true; | ||
} else { | ||
match -r { | ||
libc::ENOENT => return false, | ||
libc::EINTR => continue, | ||
err => panic!("__ulock_wake failed: {}", Error::from_raw_os_error(err)), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, are you sure this is correct? The implementation of os_sync_wake_by_address_any
seems to just call __ulock_wake
without a loop? So either we need to loop both places, or in none of them?
Same goes for futex_wake
.
We discussed this during today's T-libs meeting (albeit with low attendance). We might be willing to go along with the changes on the theory that apps are validated on current OS versions and will use current stable APIs and so shouldn't cause issues and that the fallback APIs have been available for a long time and so won't suddenly disappear on older versions. But in previous cases where we've started to rely on internal APIs we've leaned on platform experts and their investigations how risky that would be. So we'd like to do the same here. @rustbot ping apple |
Hey Apple notification group! This issue or PR could use some Apple-specific (In case it's useful, here are some instructions for tackling these sorts of cc @BlackHoleFox @hkratz @inflation @madsmtm @nvzqz @shepmaster @thomcc |
We do use internal APIs on windows (NtCreateFile), but that became necessary to fix a CVE and was used after looking at the microsoft ecosystem official projects. And windows is a more open platform in a way (hah) since they don't review most software. So it's not fully comparable. |
Just to be clear, the problematic API here is not so much the newly added APIs ( It would be nice if someone could try to submit an application for review in the App Store that uses this branch of |
Shamelessly stolen from pending PR: rust-lang/rust#122408
This is necessary to unblock rust-lang/rust#122408. The documentation for these is available [here](https://developer.apple.com/documentation/os/os_sync_wait_on_address?language=objc). Because the futex wait operations (`os_sync_wait_on_address` et al.) return the number of remaining waiters after returning, this required some changes to the common futex infrastructure, which I've changed to take a callback instead of precalculating the return values.
This is necessary to unblock rust-lang/rust#122408. The documentation for these is available [here](https://developer.apple.com/documentation/os/os_sync_wait_on_address?language=objc). Because the futex wait operations (`os_sync_wait_on_address` et al.) return the number of remaining waiters after returning, this required some changes to the common futex infrastructure, which I've changed to take a callback instead of precalculating the return values.
This is necessary to unblock rust-lang#122408. The documentation for these is available [here](https://developer.apple.com/documentation/os/os_sync_wait_on_address?language=objc). Because the futex wait operations (`os_sync_wait_on_address` et al.) return the number of remaining waiters after returning, this required some changes to the common futex infrastructure, which I've changed to take a callback instead of precalculating the return values.
Miri subtree update r? `@ghost` Unblocks rust-lang#122408 from the Miri side
Miri subtree update r? `@ghost` Unblocks rust-lang#122408 from the Miri side
Rollup merge of rust-lang#136452 - RalfJung:miri-sync, r=RalfJung Miri subtree update r? `@ghost` Unblocks rust-lang#122408 from the Miri side
Miri subtree update r? `@ghost` Unblocks rust-lang/rust#122408 from the Miri side
Miri subtree update r? `@ghost` Unblocks rust-lang/rust#122408 from the Miri side
I've removed the @rustbot ready |
As for the whole App Store situation: I consider it highly unlikely that Apple's automatic scanners will detect our usage of @rustbot label +I-libs-nominated |
We discussed this in the libs meeting. We agree that your approach is low risk, and are happy to try this out. |
☔ The latest upstream changes (presumably #136809) made this pull request unmergeable. Please resolve the merge conflicts. |
I don't think historical evidence backs you up, whatever its worth @joboet. Just as a random example, Python 3.12 started getting app's rejected because a full-text string scan of binaries contained a forbidden string name: https://lwn.net/Articles/979671/. Quoting from one of the related threads:
I'm also aware of several Mac apps which obfuscate strings in their builds to avoid the scanner problem as well, and my background radiation knowledge is that string scanning combined with symbol scanning is how a chunk of app store private API checks are done. Tis' the lib team's choice at the end of the day though and its not like temporarily not being able to upgrade Rust in an app store distributed project is the end of the world. |
Miri subtree update r? `@ghost` Unblocks rust-lang#122408 from the Miri side
So, uh, what should we do here? Roll this out and risk a revert? Wait until all supported versions supported Futexes? Obfuscate the string passed to |
I've been talking with a friend who has an app in the App Store, and got him to submit a few different versions of his app using various private symbols in different ways as a test (basically probing the black box), still waiting on the last result, give me a few weeks. |
Last week, Apple released macOS version 14.4, which introduced a public futex API called
os_sync_wait_on_address
(Apple has failed to include the documentation provided in the defining headeros/os_sync_wait_on_address.h
on its website). As the private API backing these functions has been around since macOS 10.12, our minimum supported version, it can be used as fallback without risking breakage in future versions.This PR thus switches all other synchronization primitives except
Mutex
(namelyCondvar
,RwLock
,Once
and thread parking) to the futex-based implementations also used on Linux and Windows.