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

[Dmitry Vyukov]

fixed:
http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc?r1=224755&r2=224754&pathrev=224755


On Mon, Dec 22, 2014 at 9:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Dec 22, 2014 at 1:11 AM, Dmitry Vyukov <dvyukov at google.com> wrote:
>>
>> 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:
>
>
> Maybe some issues with your grep-fu. (mine's never very good - I have to
> sanity check every time)
>
> Here's what I could find:
>
> $ grep -r "\(if\|while\|for\|switch\) ([^()]\+ [^()]\+ = " lib | wc -l
> 753
> $ grep -r "if ([^()]\+ [^()]\+ = " lib | wc -l
> 23
> blaikie at blaikie-linux:~/dev/llvm/src/projects/compiler-rt$ grep -r "if
> ([^()]\+ [^()]\+ = " lib | head
> lib/asan/asan_flags.cc:  if (const char *env = GetEnv("ASAN_OPTIONS")) {
> lib/asan/asan_stats.cc:  if (AsanThread *t = tctx->thread)
> lib/asan/asan_fake_stack.cc:  if (FakeStack *fs = GetTLSFakeStack())
> lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:      if (char
> *res = SendCommandImpl(is_data, module_name, module_offset))
> lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:      if
> (SymbolizedStack *res = libbacktrace_symbolizer_->SymbolizeCode(
> lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:      if (const
> char *demangled = libbacktrace_symbolizer_->Demangle(name))
> lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc:          if
> (const char *addr2line_path = FindPathToBinary("addr2line")) {
> lib/sanitizer_common/sanitizer_common_interceptors_format.inc:      if (void
> *argp = va_arg(aq, void *)) {
> lib/sanitizer_common/sanitizer_common_interceptors_format.inc:      if (void
> *argp = va_arg(aq, void *)) {
> lib/sanitizer_common/sanitizer_bvgraph.h:      if (uptr res = findPath(idx,
> targets, path + 1, path_size - 1))
> ...
>
>>
>>
>> $ 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;