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

Dan Liew via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 22 12:57:37 PST 2021


delcypher added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1141
+  // started.
+  if (atctx->status != ThreadStatusCreated || !asanThread)
+    return;
----------------
vitalybuka wrote:
> if we at asan_thread.cpp:260 arg is still invisible to lsan, and we can report false leak. So ThreadStatusRunning should be covered as well.
Good catch. 


================
Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1152-1167
+  // Add chunk to frontier and mark it as reachable.
+  Frontier *frontier = reinterpret_cast<Frontier *>(arg);
+  frontier->push_back(chunk);
+  LsanMetadata m(chunk);
+  m.set_tag(kReachable);
+
+  // __lsan::flags() has to be guarded by this macro because it uses data that
----------------
vitalybuka wrote:
> I would prefer we keep this logic in lsan
Yep. Your suggestion in https://reviews.llvm.org/D95183 should resolve this. One downside though is when producing log output is that we won't be able to print the thread ID. That's a pretty small downside so I'm okay with that.


================
Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1155
+  frontier->push_back(chunk);
+  LsanMetadata m(chunk);
+  m.set_tag(kReachable);
----------------
Hmm. It just occurred to me we aren't checking `m.allocated()` is true before changing the tag and adding it to the frontier. This could lead to bad behaviour if the chunk gets freed at some point (e.g. the created thread frees it). Adding a freed chunk to the frontier could be especially bad because later on when doing the flood fill in LSan we'll scan the freed memory for pointers.

I'll add a check here to prevent this.


================
Comment at: compiler-rt/lib/asan/asan_interceptors.cpp:224
+    result = REAL(pthread_create)(thread, attr, asan_thread_start, t);
   }
   return result;
----------------
vitalybuka wrote:
> if (result != 0) {
>    // destroy t;
> }
> 
Good catch. Isn't this a separate bug though? It looks like the old code didn't handle this either. I'm happy to fix it but I think it should be in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95184



More information about the llvm-commits mailing list