Page MenuHomePhabricator

[sanitizer][Darwin] GetMacosAlignedVersion() fails if sysctl is not setup
ClosedPublic

Authored by yln on Jan 6 2021, 12:56 PM.

Details

Summary

GetMacosAlignedVersion() fails for ASan-ified launchd because the
sanitizer initialization code runs before sysctl has been setup by
launchd. In this situation, sysctl kern.osproductversion returns a
non-empty string that does not match our expectations of a
well-formatted version string.

Retrieving the kernel version (via sysctl kern.osrelease) still works,
so we can use it to add a fallback for this corner case.

Diff Detail

Event Timeline

yln requested review of this revision.Jan 6 2021, 12:56 PM
yln created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2021, 12:56 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
kubamracek added inline comments.Jan 6 2021, 5:32 PM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
658

strlcpy instead?

kubamracek accepted this revision.Jan 7 2021, 6:03 PM

Can you add a comment to the header of the public function that eventually surfaced the "99.0" version (I guess that's GetMacosAlignedVersionInternal)? Something like "might return version 99 in rare cases".

With that, this looks reasonable.

This revision is now accepted and ready to land.Jan 7 2021, 6:03 PM
yln added a comment.Jan 7 2021, 6:10 PM

Can you add a comment to the header of the public function that eventually surfaced the "99.0" version (I guess that's GetMacosAlignedVersionInternal)? Something like "might return version 99 in rare cases".

With that, this looks reasonable.

Thanks! Alex and I will do one more test (Dan also suggested this): falling back to the kernel version sysctl kern.osrelease. In case this works, we can avoid returning a bogus value.

delcypher added inline comments.Jan 7 2021, 7:39 PM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
653

Maybe we should do the internal_strlen(vers) < 3 check just after the call to internal_sysctlbyname. Then if it fails set res to 1 so that we then take the fallback path that relies on the kernel version.

Although, we'd have to adapt the fallback path to support macOS 11... which makes things complicated.

yln updated this revision to Diff 316293.Jan 12 2021, 6:11 PM

Fallback to kernel version instead of returning bogus version. Add assert for launchd case getpid() == 1.

kubamracek added inline comments.Jan 12 2021, 6:17 PM
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
663

The way I'm reading this, it seems that if no_os_version is true, then vers is likely going to be an empty string too, and we'll set launchd to true (wrongly), and then fail this CHECK_EQ assert. What am I missing?

yln marked 2 inline comments as done.Jan 12 2021, 6:22 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
653

Turns out sysctl kern.osversion (kernel version) works even in launchd and this is possible!

660

You read this correctly, for launchd we have:

  • res == 0 "no error"
  • len != strlen(vers), I would consider the syscall buggy in this case. It should either return an error, or at least set the out parameters into a consistent state, e.g., len == 0 && vers == "\0".
663

Assert that we only ever encounter this crazy case for launchd.

yln edited the summary of this revision. (Show Details)Jan 12 2021, 6:23 PM
yln added a reviewer: dcoughlin.
yln marked an inline comment as done.Jan 12 2021, 6:25 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
663

Yes, of course... I am not allowed to move the CHECK_EQ here!

yln updated this revision to Diff 316298.Jan 12 2021, 6:29 PM

Fix faulty assert.

yln marked an inline comment as done.Jan 12 2021, 6:32 PM
yln added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp
663

I carelessly moved the assert if (launchd) CHECK_EQ(internal_getpid(), 1); from the block below. Thanks for catching this Kuba!

It's now guarded with "no error".

This revision was landed with ongoing or failed builds.Jan 15 2021, 11:44 AM
This revision was automatically updated to reflect the committed changes.