This is an archive of the discontinued LLVM Phabricator instance.

[Darwin] Fix OS version checks inside simulators
ClosedPublic

Authored by yln on May 14 2020, 6:55 PM.

Details

Summary

compiler-rt checks OS versions by querying the Darwin kernel version.
This is not necessarily correct inside the simulators if the simulator
runtime is not aligned with the host macOS.

Note that we still use the old code path as a fallback in case the
SIMULATOR_RUNTIME_VERSION environment variable isn't set.

rdar://63031937

Diff Detail

Event Timeline

yln created this revision.May 14 2020, 6:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2020, 6:55 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
delcypher requested changes to this revision.May 20 2020, 7:01 PM

I don't think calling os_system_version_get_current_version is safe.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
645

Calling this potentially unsafe.

For the simulators this function appears to call asprintf() (which mallocs) and free() which means we can't use this function during ASan/TSan's init. This would mean we couldn't do version checks during init which sounds like something we'd want to do.

Libxpc appears to implement this function by looking at IPHONE_SIMULATOR_ROOT in the environment and then reading a plist file rooted at that path. Reimplementing that sounds painful. Instead we could just try to parse the SIMULATOR_RUNTIME_VERSION environment variable if its set.

This revision now requires changes to proceed.May 20 2020, 7:01 PM
delcypher added inline comments.May 20 2020, 7:05 PM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
619

I'm not a huge fan of macros. Given that we only need to do this stuff inside GetMacosAlignedVersionInternal can we just do this without them?

yln updated this revision to Diff 268570.Jun 4 2020, 1:07 PM
yln marked 2 inline comments as done.

Don't call os_system_version_get_current_version(), but check SIMULATOR_RUNTIME_VERSION env var instead.

yln marked an inline comment as done.Jun 4 2020, 1:10 PM

Switched to checking the SIMULATOR_RUNTIME_VERSION env var. If it doesn't exist (which should never happen?!) we fallback to the old code path which approximates the simulator runtime version by querying the kernel version. Should we fail here instead?

If SIMULATOR_RUNTIME_VERSION is mis-formatted/lying we are out of luck: we simple interpret what's there. I think this is okay.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
645

Just to make sure I understand: we are not allowed to call anything that allocates during initialization of the sanitizers?

yln edited the summary of this revision. (Show Details)Jun 4 2020, 1:11 PM
yln marked an inline comment as done.
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
619

Trying to avoid macros/#ifdefs. Using constexpr instead.

yln updated this revision to Diff 268577.Jun 4 2020, 1:20 PM

Cleanup test code.

yln marked an inline comment as done.Jun 4 2020, 1:22 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
633

Print warning and fallback. Should we error out instead?

delcypher requested changes to this revision.Jun 9 2020, 6:11 PM
delcypher added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
611

Do we really want to fatally fail here? All is needed is for SIMULATOR_RUNTIME_VERSION to be set inappropriately and then process won't even start.

Perhaps return a boolean indicating success and we can decide in GetMacosAlignedVersionInternal() how handle failure.

619

Nice.

645

Kind of. There is a point in time during initialization where it becomes safe to call malloc but it is unsafe before then. IIRC it's safe to call malloc when Symbolizer::LateInitialize() is called or later.

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp
37

We don't build the unit tests for the simulators IIRC so I'm sure this will actually be executed.

This revision now requires changes to proceed.Jun 9 2020, 6:11 PM
yln marked 6 inline comments as done.Jun 10 2020, 9:51 AM

@delcypher: I explained my reasoning for preferring a fatal error here. Can we proceed?

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
611

Yes, I think we want a fatal error here. I think crashing on an ill-formatted env var is better than proceeding and going into a scenario that can give rise to *very* subtle bugs because the version checking off. Initially, I even thought about having a fatal error when we are in the simulator and the env var is not present at all (see comment below). I reckon you would argue agains that?

I also believe that this scenario of: 1) running in simulator, 2) env var present, 3) but ill-formatted should really never happen. Do you anticipate this happening?

Note: we still can get into a tricky situation, if the env var is well formatted, but lying. I don't think we can do anything about that.

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp
37

That's right, this is here for expressing intent (and maybe we will run the unit tests in the simulator one day?).
Any suggestions on how to better test this?

I just did an additional manual test for GetMacosAlignedVersion() with a small HelloWorld plus a Printf int the runtime to print the parsed version.

yln marked 2 inline comments as done.Jun 10 2020, 9:52 AM
yln updated this revision to Diff 277150.Jul 10 2020, 2:45 PM

Rebase/compensate for recent version scheme change (macOS 11).

yln updated this revision to Diff 277160.Jul 10 2020, 4:08 PM

Add UNREACHABLE case for macOS to GetDarwinKernelMajorFromOSMajor() to document that it is not supported.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 16 2020, 10:48 AM
This revision was automatically updated to reflect the committed changes.
delcypher added inline comments.Jul 16 2020, 11:03 AM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
636

This function name and API is leaking the implementation detail that it only needs the major. Maybe just have it take a DarwinKernelVersion argument and rename it to something like GetMacosVersionFromDarwinKernelVersion?

This change is optional.

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp
37

Given that this won't execute I think this deserves a

TODO(yln): rdar://problem/<number> Build sanitizer unit tests for the Darwin simulators.
yln added a comment.Jul 16 2020, 11:44 AM

I accidentally pushed this. Reverting and creating another revision for this.

yln marked 4 inline comments as done.Jul 16 2020, 12:05 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
636

The kernel_major for the two call sites come from different sources:

  1. GetDarwinKernelVersion().major
  2. GetEnv("SIMULATOR_RUNTIME_VERSION") (os major) -> GetDarwinKernelMajorFromOSMajor()

While there is a predictable mapping from Darwin kernel major to os major, this is not the case for the minor component. So we can't create a DarwinKernelVersion in this case.

This is also the reason why we can only really check for major OS versions (the macOS 10.x version scheme makes this a bit confusing). But we can check for <major>.<minor> kernel versions.

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp
37

I added this in the new revision.