Use futex-based synchronization on Apple platforms#122408
Use futex-based synchronization on Apple platforms#122408joboet wants to merge 2 commits intorust-lang:mainfrom
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.
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.
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.
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.
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.
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.
Yeah, we shouldn't be calling this directly, it'll trip the app store static analysis.
There was a problem hiding this comment.
I believe this should be resolved by #122408 (comment).
@joboet would you mind adding reasoning in a code comment, or at least a link to my comment?
| 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.
These panics also get to be deleted if raw syscalls are used :)
There was a problem hiding this comment.
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).
| target_os = "macos", | ||
| target_os = "ios", | ||
| target_os = "tvos", | ||
| target_os = "watchos", |
There was a problem hiding this comment.
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.
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.
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 |
|
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. |
|
Hey again. I got a friend (thanks a lot @extrawurst!) who has a live app in the Apple App Store to submit a few modified version of said app, as a way to probe whether they could pass App Store's opaque review process. Note that test 1-4 was done sometime around March IIRC, while test 5 was done ~a week ago when we picked this up again, so it is possible that the App Store changed how things work in the meantime. TheoriesThere are roughly three ways that a check like this could work:
Note that these are not mutually exclusive; the App Store may do one thing for some symbols, and another for other symbols. And they may do different things for e.g. compiled objects vs. Python files. Also note that the check could be either an allowlist or a denylist. TestsTest 1Using Codeuse core::ffi::{c_char, c_void};
use core::ptr::null_mut;
extern "C" {
fn dlsym(handle: *mut c_void, symbol: *const c_char) -> *mut c_void;
}
fn main() {
unsafe {
let _ = dlsym(null_mut(), c"__ulock_wait2".as_ptr());
let _ = dlsym(null_mut(), c"__ulock_wait".as_ptr());
let _ = dlsym(null_mut(), c"__ulock_wake".as_ptr());
}
// Rest of normal application calling `UIApplicationMain`.
}Result: Succeeded! Test 2Directly linking Codeuse core::ffi::{c_int, c_void};
use core::ptr::null_mut;
extern "C" {
fn __ulock_wait2(operation: u32, addr: *mut c_void, value: u64, timeout: u64, value2: u64) -> c_int;
fn __ulock_wait(operation: u32, addr: *mut c_void, value: u64, timeout: u32) -> c_int;
fn __ulock_wake(operation: u32, addr: *mut c_void, wake_value: u64) -> c_int;
}
fn main() {
if std::hint::black_box(false) {
__ulock_wait(0, null_mut(), 0, 0);
__ulock_wait2(0, null_mut(), 0, 0, 0);
__ulock_wake(0, null_mut(), 0);
}
// Rest of normal application calling `UIApplicationMain`.
}Result: Succeeded! Test 3Directly linking other private symbols from Codeuse core::ffi::{c_char, c_int, c_uint};
use core::ptr::null_mut;
extern "C" {
fn __openat(fd: c_int, path: *const c_char, oflag: c_int, mode: u16) -> c_int;
fn _get_cpu_capabilities() -> u64;
fn _kernelrpc_mach_vm_allocate(target: c_uint, address: *mut u32, size: u64, flags: c_int) -> c_int;
fn _dispatch_get_main_queue_port_4CF() -> c_uint;
fn dyld_dynamic_interpose(mh: *const c_void, dyld_interpose_tuple: *const c_void, count: isize);
}
fn main() {
if std::hint::black_box(false) {
let _ = __openat(0, null_mut(), 0, 0);
let _ = _get_cpu_capabilities();
let _ = _kernelrpc_mach_vm_allocate(0, null_mut(), 0, 0);
let _ = _dispatch_get_main_queue_port_4CF();
let _ = dyld_dynamic_interpose(null_mut(), null_mut(), 0);
}
// Rest of normal application calling `UIApplicationMain`.
}Result: Failed with: Test 4Directly linking private symbols from test 3 that didn't seem to fail. Codeuse core::ffi::c_int;
use core::ptr::null_mut;
extern "C" {
fn __openat(fd: c_int, path: *const c_char, oflag: c_int, mode: u16) -> c_int;
fn _get_cpu_capabilities() -> u64;
fn _kernelrpc_mach_vm_allocate(target: c_uint, address: *mut u32, size: u64, flags: c_int) -> c_int;
}
fn main() {
if std::hint::black_box(false) {
let _ = __openat(0, null_mut(), 0, 0);
let _ = _get_cpu_capabilities();
let _ = _kernelrpc_mach_vm_allocate(0, null_mut(), 0, 0);
}
// Rest of normal application calling `UIApplicationMain`.
}Result: Succeeded! Test 5Dynamically linking known private symbols from Codeuse core::ffi::{c_char, c_void};
use core::ptr::null_mut;
extern "C" {
fn dlsym(handle: *mut c_void, symbol: *const c_char) -> *mut c_void;
}
fn main() {
unsafe {
let _ = dlsym(null_mut(), c"_dispatch_get_main_queue_port_4CF".as_ptr());
let _ = dlsym(null_mut(), c"dyld_dynamic_interpose".as_ptr());
}
// Rest of normal application calling `UIApplicationMain`.
}Result: Succeeded! AnalysisIt seems to me that the App Store's checks around private-ness (at least for functions in Additionally, based on test 5, it seems like the days of All of this really is probing a black box function though. Possible failure modes:
ConclusionI believe the risk is sufficiently low that it should be safe for us to move forwards with this. Given the results above test, we could probably use weak linking for the I realized just now that I can also just try to ask Apple what they think, I have filed FB20671640 for that, will post a response here if I get one (but I think it's safe for us to move forwards with this without waiting for a response). |
|
Sorry for blocking this for so long! @rustbot author (rebase + a few unresolved comments) Also, please CC me once/if you get around to filing the follow-up that replaces |
|
|
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot label +S-waiting-on-team -S-waiting-on-author |
| } | ||
| } | ||
| } else { | ||
| panic!("your system is below the minimum supported version of Rust"); |
There was a problem hiding this comment.
This panic should be a rtabort!.
| target_os = "macos", | ||
| target_os = "ios", | ||
| target_os = "tvos", | ||
| target_os = "watchos", |
There was a problem hiding this comment.
Is there a reason this block doesn't use target_vendor = "apple" like the other blocks?
| match super::os::errno() { | ||
| // There were no waiters to wake up. | ||
| libc::ENOENT => false, | ||
| err => rtabort!("__ulock_wake failed: {}", Error::from_raw_os_error(err)), |
There was a problem hiding this comment.
| err => rtabort!("__ulock_wake failed: {}", Error::from_raw_os_error(err)), | |
| err => rtabort!("os_sync_wake_by_address_any failed: {}", Error::from_raw_os_error(err)), |
| ) | ||
| }; | ||
|
|
||
| // We promote spurious wakeups (reported as EINTR) to normal ones for |
There was a problem hiding this comment.
Is this comment accurate? The documentation doesn't list EINTR as a valid error return.
There was a problem hiding this comment.
Ah, nevermind. The documentation doesn't list this but the header does: https://gist.github.com/BlackHoleFox/00ca98a94fc75e7c48418a0685f4d050#file-os_sync_wait_on_address-h-L132
Any news on that? |
|
We discussed this in the @rust-lang/libs meeting. We are choosing to reject this as-is. Realistically, we're never going to get official permission from Apple to use a private API and we don't feel comfortable relying on it directly. Instead we recommend that the same be achieved by linking against libc++.so on Apple platforms. This is a system library (which is allowed to use private APIs) that provides |
|
People (not uncommonly) statically link libc++, even on Apple platforms. (It doesn't use the private APIs in such cases and uses a slower fallback instead, but it can provide access to newer C++ APIs than are available in the system-distributed libc++). If libstd dynamically links libc++, it might cause problems for those people. I think we really want to use the libc++ APIs, we should only do so via dlsym (against |
It seems to always use the private API and doesn't seem to have a fallback: https://github.com/llvm/llvm-project/blob/97015ad916c48ddf0fa34a4801e2851230cf5070/libcxx/src/atomic.cpp#L97 |
|
A bit of further context, I opened a Zulip thread in The takeaway there was "we need more direct contact with Apple", going through the Feedback Assistant is not the way to do this. We talked a bit about how to do so, a good suggestion for that was to try to contact some of the LLVM developers with I... said I'd do that, and I never got around to doing so, I think I was feeling discouraged by the whole situation. Sorry about that, I guess that ended up blocking this issue again :/. I still think that's a good idea, and I would still like to do it, but realistically, I'm probably not going to find the motivation for it anytime soon. So I agree with |
I'm pretty strongly against bumping the minimum OS version for various reasons (I've outlined some of them in the previous version bump in #104385). I think the best path forwards for this would be to wait for rust-lang/rfcs#3750. That would allow us to write it like: cfg_select! {
any(
target_version("macos", "14.4"),
target_version("ios", "17.4"),
target_version("tvos", "17.4"),
target_version("visionos", "1.1"),
target_version("watchos", "10.4"),
) => {
// ... os_sync_wait_on_address impl
}
_ => {
// ... current pthread impl
}
}Which would allow supporting both (though it would require Alternatively, it's possible we could have both impls in an |
Huh, you're right, I was misremembering. I could have sworn there was a check that it was part of the dylib, but it seems that while it used to be conditional, it was conditional on apple target version. Either way, this will still likely cause problems for people who statically link |
View all comments
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.hon 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,Onceand thread parking) to the futex-based implementations also used on Linux and Windows.