[asan] Allow re-exec in instrumented unit tests on Darwin (fix unit tests on macOS <=10.10)

[kubamracek]

Summary

This fixes https://llvm.org/bugs/show_bug.cgi?id=30285. On macOS 10.10 and lower, instrumented unit tests still need to be able to re-exec to make interceptors work.

[dvyukov]

+Alex, +Mike

[glider]

Sorry, I don't quite understand - do you need to enable re-exec back only for certain OSX versions? If so, shouldn't you check for them?

[kubamracek]

Sorry, I don't quite understand - do you need to enable re-exec back only for certain OSX versions? If so, shouldn't you check for them?

That's right. On 10.10 and below, we still need re-exec for unit tests to run correctly. However, it's very hard to do anything in ReexecDisabled(), because it cannot access memory (because it's instrumented and runs too early) and it can't safely call other methods (they might invoke interceptors or instrumented memory accesses). Even if we blacklisted this function, checking the version of OS X is far too complex to be done without function calls. Can we just allow this for all Darwin versions? On 10.11 and newer, re-exec doesn't happen anyway.

[kubamracek]

Ping. Is this okay to commit?

[dvyukov]

Alexander is OOO, should return soon.

[filcab]

IIUC, ReexecDisabled being false on newer systems will just make us hit DyldNeedsEnvVariable() on MaybeReexec() which will return false (on systems where we'd "prefer" to have ReexecDisabled be true), hence we'll just be removing the library path from DYLD_INSERT_VARIABLES env var (if it's there).

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

[dvyukov]

Alex, please take a look at this.

[kubamracek]

In D24699#608011, @filcab wrote:

IIUC, ReexecDisabled being false on newer systems will just make us hit DyldNeedsEnvVariable() on MaybeReexec() which will return false (on systems where we'd "prefer" to have ReexecDisabled be true), hence we'll just be removing the library path from DYLD_INSERT_VARIABLES env var (if it's there).

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

That's right. This is the reason why it's fine to have ReexecDisable() { return false; } on all macOS versions. On older OS versions, we need return false, on newer it doesn't matter.

[zaks.anna]

LGTM

For non-tests, ReexecDisabled is defined to always return false (in sanitizer_mac.cc). So with this patch, the behavior of instrumented tests will match ASan's default behavior, which brings testing setup closer to the default setup.

ReexecDisabled will only ever be returning true when we are running non-instrumented tests. The reason that is needed is that dyld uses the instrumented binary as a clue to find out if the binary is ASanified and if it should launch the process with DYLD_INTERPOSITION. The default implementation relies on this behavior, if it's not present, we need to re-exact.

[kubamracek]

If that's correct, then can we use the same trick as DyldNeedsEnvVariable for ReexecDisabled? Or am I making these two to be the same issue when they aren't?

Do you mean to check dyldVersionNumber? That would just duplicate the code. How about I just document this more in a comment and keep it as return false (for instrumented tests on macOS)?

[kubamracek]

No need to duplicate if you export the function or make it online anyway.

I don't think we should publish DyldNeedsEnvVariable() as public interface of the sanitizer dylibs. And instrumented tests can't call non-public functions.

But having the comment makes sense. I'm ok with this.

I'll just try to describe this better in a comment. Thanks!

compiler-rt/trunk/lib/asan/tests/asan_test_main.cc

#if SANITIZER_MAC
   return "symbolize=false:abort_on_error=0:log_to_syslog=0";
 #else
   return "symbolize=false";
 #endif
 }
 
 namespace __sanitizer {
 bool ReexecDisabled() {
+#if __has_feature(address_sanitizer) && SANITIZER_MAC
+  // Allow re-exec in instrumented unit tests on Darwin.  Technically, we only
+  // need this for 10.10 and below, where re-exec is required for the
+  // interceptors to work, but to avoid duplicating the version detection logic,
+  // let's just allow re-exec for all Darwin versions.  On newer OS versions,
+  // returning 'false' doesn't do anything anyway, because we don't re-exec.
+  return false;
+#else
   return true;
+#endif
 }
-}
+}  // namespace __sanitizer
 
 int main(int argc, char **argv) {
   testing::GTEST_FLAG(death_test_style) = "threadsafe";
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }