[PATCH] D15380: [tsan] Update dispatch_group support to avoid using a disposed group object

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:93
@@ -90,2 +92,3 @@
   user_free(thr, pc, context);
+  if (context->object_to_release) dispatch_release(context->object_to_release);
 }
----------------
kubabrecka wrote:
> dvyukov wrote:
> > dvyukov wrote:
> > > dvyukov wrote:
> > > > Looks like use-after-free. You've just freed the context line above.
> > > Please add a comment somewhere here explaining why we need this additional lifetime management.
> > I don't understand the exact scenario that leads to the problem. The group is context->object_to_acquire, which we touch in the very beginning of this function. Then we call user callback orig_work. In your tests orig_work calls dispatch_group_leave on the group. So either (1) test code is also buggy, or (2) we don't have use-after-free in this function and don't need this change, or (3) I am missing something.
> > 
> > In your tests orig_work calls dispatch_group_leave
> No, orig_work here is the empty block (or empty function) passed as argument to dispatch_group_notify (or dispatch_group_notify_f).  The issue is that the API doesn't guarantee that the group itself will still be alive, when the notification is invoked.
> 
> > Looks like use-after-free. You've just freed the context line above.
> Oops, you're right.
I've missed that it is only for dispatch_group_notify.
Then, I think you need to move the release right below the Acquire and add a comment.

As a side note, it can make sense to detect such bugs in user code. If you add MemoryRead/Write in necessary places, tsan will be able to detect such racy-use-after-free usages. You can see tsan_rtl_mutex.cc for an example.



http://reviews.llvm.org/D15380