microsoft / mimalloc

mimalloc is a compact general purpose allocator with excellent performance.
MIT License
9.74k stars 793 forks source link

prim.c: add /dev/urandom fallback for old OS X #829

Closed jmroot closed 4 months ago

jmroot commented 7 months ago

arc4random_buf is not always available. Also changed MAC_OS_X_VERSION_MAX_ALLOWED check to MAC_OS_X_VERSION_MIN_REQUIRED, because no runtime checking of either the OS version or the presence of weak-linked symbols is being done, so runtime behaviour would be incorrect if you built against the latest SDK and deployed to an older OS version.

jmroot commented 7 months ago

@microsoft-github-policy-service agree

daanx commented 4 months ago

Thanks! I pushed a fix to dev (merge with dev-slice soon). Can you test it?

jmroot commented 4 months ago

@daanx I haven't been able to try builds yet, but the changes mostly look good, with the exception of the OS version checks. I don't know how familiar you are with Apple's availability checking system, so I'll give some background.

MAC_OS_X_VERSION_MAX_ALLOWED corresponds to the SDK you are building against. That of course determines which interfaces are available at compile time. MAC_OS_X_VERSION_MIN_REQUIRED is the oldest OS version that the binary being built will be able to run on. That determines which interfaces can be known to be available at runtime without further checking, and can be changed by setting MACOSX_DEPLOYMENT_TARGET in the environment or by using the -mmacosx-version-min option with clang.

For interfaces that were introduced in an OS version newer than MAC_OS_X_VERSION_MIN_REQUIRED, the binary will be weak-linked with the corresponding symbols. That means that the dynamic linker will bind them at runtime if they exist, but ignore them if they don't. Using an unbound symbol will cause a crash. So, you have to check for potentially weak-linked symbols at runtime before using them. With recent clang versions, __builtin_available is the recommended way of doing this, while with older compilers you just check if the symbol is NULL.

What all this means for the code at hand is that if you don't do any runtime availability checking, it is safe to use interfaces iff both MAC_OS_X_VERSION_MAX_ALLOWED and MAC_OS_X_VERSION_MIN_REQUIRED are >= the version they were introduced in. For arc4random_buf that's 1070 (10.7) and for CCRandomGenerateBytes it's 101000 (10.10).

daanx commented 4 months ago

Thanks for the explanation! I updated the checks -- can you see if it looks good to you?

barracuda156 commented 4 months ago

@daanx This will not work: defined(MAC_OS_X_VERSION_10_7) && (MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_7), since MAC_OS_X_VERSION_10_7 is not defined on macOS < 10.7.

This is perhaps the correct way: !defined(MAC_OS_X_VERSION_10_7) || (MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_7)

P. S. Comparing against numerical versions works fine: MAC_OS_X_VERSION_MIN_REQUIRED < 1070.

barracuda156 commented 4 months ago

Just for the sake of completeness,

and can be changed by setting MACOSX_DEPLOYMENT_TARGET in the environment or by using the -mmacosx-version-min option with clang.

-mmacosx-version-min works with GCC too, not only Clang.

With recent clang versions, __builtin_available is the recommended way of doing this, while with older compilers you just check if the symbol is NULL.

AFAIK, __builtin_available is a clangism, so cannot be used with modern compilers as well, if those are not Clang.

jmroot commented 4 months ago

Thanks for the explanation! I updated the checks -- can you see if it looks good to you?

There is indeed the problem that MAC_OS_X_VERSION_10_7 is undefined on systems older than 10.7. Otherwise looks correct, assuming the intent is to only use CCRandomGenerateBytes on 10.15 and later due to the performance issue on older OS versions (not sure if that is considered worse than the potential error handling issue with arc4random_buf.)

daanx commented 4 months ago

Thanks for the quick follow up! I yet pushed another fix -- hope it is correct now. I think we just stick with CCGetRandomBytes as it is the "best" source.

jmroot commented 4 months ago

@daanx The dev branch builds on macOS 10.7 now. Still a couple of issues on 10.6, which I've opened PRs for: #859 #860