[compiler-rt] r219600 - tsan: better reporting for virtual-call-after-free

[Dmitry Vyukov]

On Mon, Oct 13, 2014 at 7:02 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Oct 13, 2014 at 1:46 AM, Dmitry Vyukov <dvyukov at google.com> wrote:
>>
>> Author: dvyukov
>> Date: Mon Oct 13 03:46:25 2014
>> New Revision: 219600
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=219600&view=rev
>> Log:
>> tsan: better reporting for virtual-call-after-free
>> Previously we said that it's a data race, which is confusing
>> if it happens in the same thread.
>>
>>
>>
>> Added:
>>     compiler-rt/trunk/test/tsan/vptr_harmful_race4.cc
>> Modified:
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_report.h
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc
>>     compiler-rt/trunk/lib/tsan/rtl/tsan_suppressions.cc
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc?rev=219600&r1=219599&r2=219600&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_report.cc Mon Oct 13 03:46:25 2014
>> @@ -70,6 +70,8 @@ static const char *ReportTypeString(Repo
>>      return "data race on vptr (ctor/dtor vs virtual call)";
>>    if (typ == ReportTypeUseAfterFree)
>>      return "heap-use-after-free";
>> +  if (typ == ReportTypeVptrUseAfterFree)
>> +    return "heap-use-after-free (virtual call vs free)";
>>    if (typ == ReportTypeThreadLeak)
>>      return "thread leak";
>>    if (typ == ReportTypeMutexDestroyLocked)
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_report.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_report.h?rev=219600&r1=219599&r2=219600&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_report.h (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_report.h Mon Oct 13 03:46:25 2014
>> @@ -22,6 +22,7 @@ enum ReportType {
>>    ReportTypeRace,
>>    ReportTypeVptrRace,
>>    ReportTypeUseAfterFree,
>> +  ReportTypeVptrUseAfterFree,
>>    ReportTypeThreadLeak,
>>    ReportTypeMutexDestroyLocked,
>>    ReportTypeMutexDoubleLock,
>>
>> 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=219600&r1=219599&r2=219600&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_rtl_report.cc Mon Oct 13 03:46:25
>> 2014
>> @@ -627,7 +627,9 @@ void ReportRace(ThreadState *thr) {
>>    ThreadRegistryLock l0(ctx->thread_registry);
>>
>>    ReportType typ = ReportTypeRace;
>> -  if (thr->is_vptr_access)
>> +  if (thr->is_vptr_access && freed)
>> +    typ = ReportTypeVptrUseAfterFree;
>> +  else if (thr->is_vptr_access)
>>      typ = ReportTypeVptrRace;
>>    else if (freed)
>>      typ = ReportTypeUseAfterFree;
>>
>> Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_suppressions.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_suppressions.cc?rev=219600&r1=219599&r2=219600&view=diff
>>
>> ==============================================================================
>> --- compiler-rt/trunk/lib/tsan/rtl/tsan_suppressions.cc (original)
>> +++ compiler-rt/trunk/lib/tsan/rtl/tsan_suppressions.cc Mon Oct 13
>> 03:46:25 2014
>> @@ -60,6 +60,8 @@ SuppressionType conv(ReportType typ) {
>>      return SuppressionRace;
>>    else if (typ == ReportTypeUseAfterFree)
>>      return SuppressionRace;
>> +  else if (typ == ReportTypeVptrUseAfterFree)
>> +    return SuppressionRace;
>>    else if (typ == ReportTypeThreadLeak)
>>      return SuppressionThread;
>>    else if (typ == ReportTypeMutexDestroyLocked)
>>
>> Added: compiler-rt/trunk/test/tsan/vptr_harmful_race4.cc
>> URL:
>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/tsan/vptr_harmful_race4.cc?rev=219600&view=auto
>>
>> ==============================================================================
>> --- compiler-rt/trunk/test/tsan/vptr_harmful_race4.cc (added)
>> +++ compiler-rt/trunk/test/tsan/vptr_harmful_race4.cc Mon Oct 13 03:46:25
>> 2014
>> @@ -0,0 +1,34 @@
>> +// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
>> +#include <pthread.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +
>> +struct A {
>> +  virtual void F() {
>> +  }
>> +
>> +  virtual ~A() {
>> +  }
>> +};
>> +
>> +struct B : A {
>> +  virtual void F() {
>> +  }
>> +};
>> +
>> +void *Thread(void *x) {
>> +  sleep(1);
>> +  ((A*)x)->F();
>> +  return 0;
>> +}
>> +
>> +int main() {
>> +  A *obj = new B;
>> +  pthread_t t;
>> +  pthread_create(&t, 0, Thread, obj);
>> +  delete obj;
>> +  pthread_join(t, 0);
>> +}
>> +
>> +// CHECK: WARNING: ThreadSanitizer: heap-use-after-free (virtual call vs
>> free)
>
>
> Could/should we be more accurate, or at least more vague, about which kind
> of allocation was done? (this example uses 'delete', yet the diagnostic says
> 'free') "virtual call after deallocation"? Not sure, maybe free is
> sufficient. Just a thought.


I would prefer to not do it proactively. It's lots of work for little
benefit. For this particular things (data race in single thread),
we've got an explicit user complaint.

Even asan always says use-after-free. If we decide that it's important
we need to start with asan.