[Sanitizers] Order initialization of coverage and guard arrays

[kutuzov.viktor.84]

Summary

Since there's no guarantee the first InitializeGuardArray() call comes after Init(), we have to make sure the coverage is initialized before we add a guard array.

[kcc]

Since there's no guarantee the first InitializeGuardArray() call comes after Init(), we have to make sure the coverage is initialized before we add a guard array.

Could you please remind us why we can't guarantee this?
(Better yet, add a test)

[kutuzov.viktor.84]

Could you please remind us why we can't guarantee this?

Well, we probably can, but it seems it's not implemented for all cases. :-)

The instrumentation part of coverage (Instrumentation/SanitizerCoverage.cpp) generates module-scope global objects and puts calls to __sanitizer_cov_module_init() that is responsible for initializing module's guards to the ctors of these global objects. Similarly, __ubsan::InitIfNecessary() that takes care of the initialization of coverage (lib/ubsan/ubsan_init.cc) is either part of the .preinit_array or called from the ctor of another global object (ubsan_initializer)--depending on whether the SANITIZER_CAN_USE_PREINIT_ARRAY macro is defined. In the latter case, since these objects belong to different modules, they can generally be initialized in any order.

(Better yet, add a test)

Umm, I guess for this we would need to replace the ..._PREINIT_ARRAY macro with a run-time flag that controls the way the initialization is performed. Do we really want this?

[kcc]

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

186

Maybe just call Init() here?

[kutuzov.viktor.84]

Updated.

[kcc]

LGTM

[eugenis]

There is actually a bigger problem here: if sanitizer_cov_module_init runs before asan_init (and that's possible since they are at the same priority), we will fail to activate asan, in particular - we won't update coverage and coverage_dir from activation flags.

I think the right fix would be reverting this change and lowering the priority of the coverate module initializer to 2 (keeping __asan_init at 1).

[kutuzov.viktor.84]

I think the right fix would be reverting this change and lowering the priority of the coverate module initializer to 2 (keeping __asan_init at 1).

AFAIU, priorities order initializers associated with the same module. If that is correct, then we couldn't fix this specific issue this way.

[eugenis]

Priorities are per-DSO.
Constructors are emitted into numbered sections and sorted in the linker.

[kutuzov.viktor.84]

OK, it makes sense, thanks. However, while it works for Asan's __asan_init()t called from the code injected by the instrumentation part, it doesn't work for UBsan where, in omission of SANITIZER_CAN_USE_PREINIT_ARRAY, __ubsan::InitIfNecessary() is called from a regular constructor which has a lower priority than __sanitizer_cov_module_init. Maybe we should introduce instrumentation for UBsan or just replace the ubsan_initializer declaration:

class UbsanInitializer {
 public:
  UbsanInitializer() {
    InitIfNecessary();
  }
};
static UbsanInitializer ubsan_initializer;

with something like:

__attribute__((constructor(1)))
void __init_ubsan() {
  InitIfNecessary();
}

[eugenis]

It sounds like __sancov module constructor should have the same logic as, for example, asan interceptors: initialize asan if not initialized yet. We could have a hook to "initialize the current sanitizer" available in sanitizer_common and call that.

This problem is tied to the concept of "current sanitizer" and the initialization order of UBSan which is currently very poorly defined (see ubsan init trying to guess if any other sanitizer has been initialized yet).

[kutuzov.viktor.84]

OK, it seems to be a design-level issue and needs some investigation. The question is: can we re-apply rL226440 for now to make things working with the existing UBsan tests on FreeBSD? Or, would you like me to prepare a patch introducing the ENSURE_UBSAN_INITED() kind of things? Thanks.

[eugenis]

I think it's fine to reapply the original fix for now.

[kutuzov.viktor.84]

Re-applied as rL227789.

lib/sanitizer_common/sanitizer_coverage_libcdep.cc

void CoverageData::DirectOpen() {
   CovUpdateMapping(coverage_dir);
 }
 
 void CoverageData::Init() {
   pc_fd = kInvalidFd;
 }
 
 void CoverageData::Enable() {
-  CHECK_EQ(pc_array, nullptr);
+  if (pc_array)
+    return;
   pc_array = reinterpret_cast<uptr *>(
       MmapNoReserveOrDie(sizeof(uptr) * kPcArrayMaxSize, "CovInit"));
   atomic_store(&pc_array_index, 0, memory_order_relaxed);
   if (common_flags()->coverage_direct) {
     atomic_store(&pc_array_size, 0, memory_order_relaxed);
   } else {
     atomic_store(&pc_array_size, kPcArrayMaxSize, memory_order_relaxed);
   }
tr_event_array = reinterpret_cast<u32 *>(MmapNoReserveOrDie(
       "CovInit::tr_event_array"));
   Mprotect(reinterpret_cast<uptr>(&tr_event_array[kTrEventArrayMaxSize]),
            GetMmapGranularity());
   tr_event_array_size = kTrEventArrayMaxSize;
   tr_event_pointer = tr_event_array;
 }
 
 void CoverageData::InitializeGuardArray(s32 *guards) {
+  Enable();  // Make sure coverage is enabled at this point.
   s32 n = guards[0];
   for (s32 j = 1; j <= n; j++) {
     uptr idx = atomic_fetch_add(&pc_array_index, 1, memory_order_relaxed);
     guards[j] = -static_cast<s32>(idx + 1);
   }
 }
 
 void CoverageData::Disable() {