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

[David Blaikie]

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;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141222/d010b6a8/attachment.html>