[tsan] Assert to make sure we don't try to Acquire() or Release() a NULL pointer

[kubamracek]

Summary

This will provide much better diagnostics when we try to Acquire(NULL) or Release(NULL). I've been recently debugging such a bug and this would have helped tremendously.

[dvyukov]

What was the bug? How did it manifest?
I somewhat afraid of breaking some existing code. Currently we consider memory around 0 to be "user memory". And in fact it's possible to mmap the first page at least on some linuxes, not sure about other OSes. And there is some effort to port sanitizers to mmu-less environment (https://reviews.llvm.org/D30583). Another possible use if you have N virtual entities and use N as address passed to acquire/release annotations (not perfect, but currently valid).
If we are excluding NULL from user memory, then I think we need to change IsAppMem to return false for NULL. That will make behavior consistent across all functions. Acquire/Release already check IsAppMem for the passed address (at least in debug build). It will also make simpler to undo this decision in some particular contexts.

[kubamracek]

What was the bug? How did it manifest?

The bug was that a GCD API (dispatch_after) was called with NULL as a queue, and our interceptors then did Acquire(NULL). This caused a segfault within MetaMap and was hard to reproduce (this was unrelated to the bug, the bug was straightforward).

Right, I can see that 0 can be valid user memory in some cases. How about defining a boolean in tsan_platform.h that simply says whether NULL is valid user memory or not? We could set it just for Linux and macOS at this point. Or are you saying that even on these systems we should support when someone explicitly mmaps something into the 0 page?

[dvyukov]

I see that we already exclude the first page from app memory on x86_64:

static const uptr kLoAppMemBeg   = 0x000000001000ull;

So I think a better check would be:

CHECK(IsAppMem(addr));

It will also check for any other bogus addresses, like random values, NULL+epsilon (&null_ptr->field) and also will not duplicate mapping logic.

lib/tsan/rtl/tsan_rtl_mutex.cc

void MutexInvalidAccess(ThreadState *thr, uptr pc, uptr addr) {
   SyncVar *s = ctx->metamap.GetOrCreateAndLock(thr, pc, addr, true);
   u64 mid = s->GetId();
   s->mtx.Unlock();
   ReportMutexMisuse(thr, pc, ReportTypeMutexInvalidAccess, addr, mid);
 }
 
 void Acquire(ThreadState *thr, uptr pc, uptr addr) {
   DPrintf("#%d: Acquire %zx\n", thr->tid, addr);
+  CHECK(addr && "addr must not be NULL");
   if (thr->ignore_sync)
     return;
   SyncVar *s = ctx->metamap.GetIfExistsAndLock(addr, false);
   if (!s)
     return;
   AcquireImpl(thr, pc, &s->clock);
   s->mtx.ReadUnlock();
 }
if (thr->ignore_sync)
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   ctx->thread_registry->RunCallbackForEachThreadLocked(
       UpdateClockCallback, thr);
 }
 
 void Release(ThreadState *thr, uptr pc, uptr addr) {
   DPrintf("#%d: Release %zx\n", thr->tid, addr);
+  CHECK(addr && "addr must not be NULL");
   if (thr->ignore_sync)
     return;
   SyncVar *s = ctx->metamap.GetOrCreateAndLock(thr, pc, addr, true);
   thr->fast_state.IncrementEpoch();
   // Can't increment epoch w/o writing to the trace as well.
   TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
   ReleaseImpl(thr, pc, &s->clock);
   s->mtx.Unlock();
 }
 
 void ReleaseStore(ThreadState *thr, uptr pc, uptr addr) {
   DPrintf("#%d: ReleaseStore %zx\n", thr->tid, addr);
+  CHECK(addr && "addr must not be NULL");
   if (thr->ignore_sync)
     return;
   SyncVar *s = ctx->metamap.GetOrCreateAndLock(thr, pc, addr, true);
   thr->fast_state.IncrementEpoch();
   // Can't increment epoch w/o writing to the trace as well.
   TraceAddEvent(thr, thr->fast_state, EventTypeMop, 0);
   ReleaseStoreImpl(thr, pc, &s->clock);
   s->mtx.Unlock();