[compiler-rt] r224508 - tsan: don't crash with NULL deref during reporting

[Dmitry Vyukov]

On Mon, Dec 22, 2014 at 5:28 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Thu, Dec 18, 2014 at 2:19 AM, Dmitry Vyukov <dvyukov at google.com> wrote:
>>
>> Author: dvyukov
>> Date: Thu Dec 18 04:19:32 2014
>> New Revision: 224508
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=224508&view=rev
>> Log:
>> tsan: don't crash with NULL deref during reporting
>>
>> tctx==NULL crash observed during deadlock reporting.
>> There seems to be some bugs in the deadlock detector,
>> but it is still useful to be more robust during reporting.
>
>
> So you don't know why tctx is null in this case - isn't this a bit of a
> hack/workaround, rather than an actual fix, then? It's sort of unfortunate
> to build up cruft to tolerate violations of invariants we believe should
> hold...

The comment is not very precise. It is not a bug in deadlock detector,
whenever we report something there are chances that involved threads
and mutexes are already destroyed and are forget. In other cases we
check for NULL in callers. So this check is exactly what we want.


>> Modified:
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc?rev=224508&r1=224507&r2=224508&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc Thu Dec 18 04:19:32
>> 2014
>> @@ -251,7 +251,9 @@ ThreadContext *IsThreadStackOrTls(uptr a
>>
>>  void ScopedReport::AddThread(int unique_tid, bool suppressable) {
>>  #ifndef SANITIZER_GO
>> -  AddThread(FindThreadByUidLocked(unique_tid), suppressable);
>> +  const ThreadContext *tctx = FindThreadByUidLocked(unique_tid);
>> +  if (tctx)
>
>
> Usually we roll the variable declaration into the condition in cases like
> this.

See no single instance of declaring variables in if's:

$ egrep "if \((uptr|int|unsigned|u8|s8)\ .*\)" rtl/*.cc
../sanitizer_common/*.cc ../asan/*.cc ../msan/*.cc
$ egrep "if \(.*\*\)" rtl/*.cc ../sanitizer_common/*.cc ../asan/*.cc
../msan/*.cc
rtl/tsan_interceptors.cc:    if ((res = ((const unsigned char *)s1)[len] -
rtl/tsan_interceptors.cc:    if
(pthread_setspecific(g_thread_finalize_key, (void*)(iter - 1))) {
rtl/tsan_rtl_report.cc:  if (a->PointerIsMine((void*)addr)) {
rtl/tsan_sync.cc:    if
(atomic_compare_exchange_strong((atomic_uint32_t*)meta, &idx0,
../sanitizer_common/sanitizer_libc.cc:  if ((char*)aligned_end >= beg)
../sanitizer_common/sanitizer_linux.cc:    if ((char *)entry_ >=
&buffer_[bytes_read_] && !GetDirectoryEntries())
../sanitizer_common/sanitizer_procmaps_mac.cc:  if (((const
load_command *)lc)->cmd == kLCSegment) {
../asan/asan_allocator.cc:    if (*(u8
*)MEM_TO_SHADOW((uptr)allocated) == 0 && CanPoisonMemory()) {
../asan/asan_allocator.cc:    if
(!atomic_compare_exchange_strong((atomic_uint8_t*)m, &old_chunk_state,
../asan/asan_report.cc:  if (*(char*)(g.beg + g.size - 1) != '\0') return;
../asan/asan_rtl.cc:  if (res != (void*)beg) {
../msan/msan_interceptors.cc:  if (res != (void*)-1)
../msan/msan_interceptors.cc:  if (res != (void*)-1)
../msan/msan_interceptors.cc:  if (p != (void *)-1) {
../msan/msan_interceptors.cc:    if (!((MSanInterceptorContext
*)ctx)->in_interceptor_scope) \
../msan/msan_interceptors.cc:  if (map) ForEachMappedRegion((link_map
*)map, __msan_unpoison);
../msan/msan_interceptors.cc:    if (*(u8 *)src_s) *(u32
*)SHADOW_TO_ORIGIN(dst_s &~3UL) = src_origin;
../msan/msan_linux.cc:    if (shadow != (void*)kShadowBeg) return false;
../msan/msan_linux.cc:    if (origins != (void*)kOriginsBeg) return false;