[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

[Nicholas Allegra via Phabricator via lldb-commits]

comex added a comment.

In D86388#2234418 <https://reviews.llvm.org/D86388#2234418>, @jingham wrote:

> I'm confused as to how this patch actually fixes the problem.  When the thread gets removed from the thread list, it should get Destroy called on it - which should set m_destroy_called, causing IsValid to return false..  So I am not clear under what circumstances FindThreadByID will fail, but the cached thread shared pointer's IsValid is still true?  If IsValid is holding true over the thread's removal from the thread list, then I'm worried that this change will keep us using the old ThreadSP that was reported the next time we stopped and this thread ID was represented by a different ThreadSP.

Hmm… I’m confused by your comment.  This patch isn’t meant to address a situation where IsValid is true but FindThreadByID fails.  It addresses the situation where a ThreadPlan already has a cached thread pointer (and would like to reuse it without calling FindThreadByID at all), but IsValid is false because the thread was removed from the thread list since it was cached.

The existing code doesn’t check IsValid; it assumes that the thread list can’t be changed until a resume happens, at which point WillResume will be called and reset the cached thread pointer to null.  However, in the buggy case I found, GetThread is called again after WillResume has already run.  GetThread sets the cached thread pointer back to non-null, and then later when the thread list actually changes, the pointer becomes dangling.

With this patch, the pointer never gets reset to null, so it can end up pointing to a thread that has been removed from the list.  But now it’s a shared_ptr, so it at least keeps the thread object alive.  And every time GetThread is called, it checks (using IsValid) whether the thread has been removed.  If so, GetThread throws out the cached pointer and falls back to calling FindThreadByID.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86388