[PATCH] D94210: [ASan] Stop blocking child thread progress from parent thread in `pthread_create` interceptor.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_interceptors.cpp:211
+  u32 current_tid = GetCurrentTidOrInvalid();
+  AsanThread *t =
+      AsanThread::Create(start_routine, arg, current_tid, &stack, detached);
----------------
previously we kept AsanThread only if pthread_create succeed.
now if pthread_create fails we still keep it. We probably should destroy it.



================
Comment at: compiler-rt/lib/asan/asan_interceptors.cpp:220
+    //
+    // We set a flag to tell the child thread if it should unignore the object.
+    // We should not try to unignore the object if `kIgnoreObjectAlreadyIgnored`
----------------
delcypher wrote:
> delcypher wrote:
> > vitalybuka wrote:
> > > vitalybuka wrote:
> > > > probably too artificial but looks like will be broken
> > > > 
> > > > ```
> > > > p = new char;
> > > > pthread_create(... p)
> > > > delete p;  // we know that thread is not going to use it
> > > > ... 
> > > > 
> > > > p = new char; // unrelated allocation
> > > > Ingore(p)
> > > > ```
> > > > 
> > > > could be simple and more reliable to change leak sanitizer to walk through all alive threads and include thread->arg_ into frontier?
> > > more practical
> > > 
> > > ```
> > > shared = new char;
> > > pthread_create(... shared)
> > > pthread_create(... shared)
> > > ```
> > > 
> > > if the first thread unignores and exists and the second thread are not yet started we may see false leak.
> > > should be fine with proposed lsan change.
> > > 
> > For the benefit of readers, here's what can go wrong in the "For the "we know that thread is not going to use it" example.
> > 
> > Note we are assuming that the second `new char` gets the same address as the first `new char`. This wouldn't happen with the quarantine in place which is not the normal runtime configuration so this example is a bit artificial but it is still worth considering.
> > 
> > If the call to `Ingore(p)` (i.e. `__lsan_ignore_object()`) happens in between the call to `__lsan::IgnoreObjectLocked()` in the pthread_create interceptor and the call to `__lsan::UnIgnoreObjectLocked()` in the child thread then it will be as if that call never happened because the child thread will unignore the object regardless of the `Ingore(p)` call.
> Thanks for the examples. They illustrate a significant problem with my approach that I hadn't considered.
> 
> Your proposal to have "leak sanitizer to walk through all alive threads and include thread->arg_ into frontier" would work if we could "walk through all alive threads". Unfortunately we can't do that with the removal of the blocking behaviour that this patch tries to achieve.
> 
> This is because although we create the `AsanThread` object in the parent thread (in the pthread_create interceptor) we don't actually register this thread  with the thread registry until the child thread starts (registration happens in the call to `t->ThreadStart(GetTid());` in the child thread). Because the thread isn't registered with the thread registry in between the exiting of the `pthread_create()` interceptor and the start of the child thread LSan won't be able to find this unstarted child thread in the thread registry and thus can't ignore its `arg`.
> 
> However, I have two alternative ideas that might work...
> 
> **Solution 1.** Introduce internal APIs that can be used to add/remove pointers from the frontier. Then this patch would call `__lsan::AddPtrToFrontier()` and `__lsan::RemoveAddedPtrFromFrontier()` in the pthread_create interceptor (parent thread) and in `asan_thread_start()` (child thread). The idea here being if leak detection is triggered in between `__lsan::AddPtrToFrontier()` and `__lsan::RemoveAddedPtrFromFrontier()` the `arg` of the not started child thread will be added to the frontier when the frontier is created and thus reporting the `arg` leak will be suppressed.
> 
> We need to do `__lsan::RemoveAddedPtrFromFrontier()` because that pointer might not be valid forever and the address could get reused for something which would mess up reporting leaks.
> It looks like the `Frontier` object stores `LsanMetadata` chunks so these new APIs would need to ignore pointers that are not allocated chunks.
> 
> **Solution 2.**
> 
> Change the code to basically not do the blocking behaviour when leak checking is off. This **technically** gets me to my end goal because the situation I care about is where leak checking is off. I don't really like this solution though because it
> 
> * Make the code complicated.
> * Creates a divergence in behaviour between leaks on vs leaks off which we probably don't have good test coverage for today (most tests probably run with leak checking on).
> * Doesn't fix the performance problem when leak checking is on.
> 
> What do you think? Do you like either of these solutions or do you have another you'd like to propose?
> Your proposal to have "leak sanitizer to walk through all alive threads and include thread->arg_ into frontier" would work if we could "walk through all alive threads". Unfortunately we can't do that with the removal of the blocking behaviour that this patch tries to achieve.

Your patch still calls AsanThread::Create -> asanThreadRegistry().CreateThread -> adds args into threads_
We don't rely care if it's alive or not. Leak sanitizer can add all args from there into frontier. When thread exists we can set args to null, just in case.


> Solution 1.
LGTM, if the one above is not good enough.
However lets call it something like lsan::IgnorePermanently or AddToIgnoreList avoid exposing "Frontier" which is internal lsan term and to show somehow that it's different from IgnoreObjectLocked that ignore does not goes away when object is deallocated.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94210/new/

https://reviews.llvm.org/D94210