[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.Background:ThreadPlan objects store a cached pointer to the associated Thread. To quotethe code:// We don't cache the thread pointer over resumes. This// Thread might go away, and another...

[Nicholas Allegra via Phabricator via lldb-commits]

comex created this revision.
comex added reviewers: jingham, clayborg.
Herald added a subscriber: aaron.ballman.
Herald added a project: LLDB.
comex requested review of this revision.
Herald added a subscriber: JDevlieghere.

...Thread represent
// the same underlying object on a later stop.

This can happen only when using an operating system plugin with
os-plugin-reports-all-threads = false (a new feature); otherwise, the
ThreadPlan will be wiped away when the Thread is.

Previously, this cached pointer was unowned, and ThreadPlan attempted to
prevent it from becoming stale by invalidating it in WillResume, reasoning that
the list of threads would only change whwen the target is running.  However, it
turns out that the pointer can be re-cached after it's invalidated but before
the target actually starts running.  At least one path where this happens is
ThreadPlan::ShouldReportRun -> GetPreviousPlan -> GetThread.

It might be possible to fix this by invalidating the pointer from other places,
but that seems unnecessarily risky and complicated.  Instead, just keep around
a ThreadSP and check IsValid(), which becomes false when Thread::DestroyThread()
is called.

Note: This does not create a retain cycle because Thread does not own
ThreadPlans.  (Even if it did, Thread::DestroyThread resets all of the thread's
owned pointers.)

As for testing, I made a small change to the existing reports-all-threads test
which causes it to trigger the use-after-free without the rest of the commit.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86388

Files:
  lldb/include/lldb/Target/ThreadPlan.h
  lldb/source/Target/ThreadPlan.cpp
  lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py


Index: lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py
===================================================================
--- lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py
+++ lldb/test/API/functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py
@@ -51,6 +51,13 @@
         (target, self.process, thread, thread_bkpt) = lldbutil.run_to_source_breakpoint(
             self, "first stop in thread - do a step out", self.main_file)
 
+        # Disabling the thread breakpoint here ensures that we don't have an
+        # unreported stop (to step over that breakpoint), which ensures that
+        # ThreadPlan::ShouldReportRun is called in between ThreadPlan::WillResume
+        # and when the thread actually disappears.  This previously triggered
+        # a use-after-free.
+        thread_bkpt.SetEnabled(False)
+
         main_bkpt = target.BreakpointCreateBySourceRegex('Stop here and do not make a memory thread for thread_1',
                                                          self.main_file)
         self.assertEqual(main_bkpt.GetNumLocations(), 1, "Main breakpoint has one location")
Index: lldb/source/Target/ThreadPlan.cpp
===================================================================
--- lldb/source/Target/ThreadPlan.cpp
+++ lldb/source/Target/ThreadPlan.cpp
@@ -24,10 +24,10 @@
     : m_process(*thread.GetProcess().get()), m_tid(thread.GetID()),
       m_stop_vote(stop_vote), m_run_vote(run_vote),
       m_takes_iteration_count(false), m_could_not_resolve_hw_bp(false),
-      m_thread(&thread), m_kind(kind), m_name(name), m_plan_complete_mutex(),
-      m_cached_plan_explains_stop(eLazyBoolCalculate), m_plan_complete(false),
-      m_plan_private(false), m_okay_to_discard(true), m_is_master_plan(false),
-      m_plan_succeeded(true) {
+      m_thread_sp(thread.shared_from_this()), m_kind(kind), m_name(name),
+      m_plan_complete_mutex(), m_cached_plan_explains_stop(eLazyBoolCalculate),
+      m_plan_complete(false), m_plan_private(false), m_okay_to_discard(true),
+      m_is_master_plan(false), m_plan_succeeded(true) {
   SetID(GetNextID());
 }
 
@@ -39,12 +39,11 @@
 const Target &ThreadPlan::GetTarget() const { return m_process.GetTarget(); }
 
 Thread &ThreadPlan::GetThread() {
-  if (m_thread)
-    return *m_thread;
+  if (m_thread_sp && m_thread_sp->IsValid())
+    return *m_thread_sp;
 
-  ThreadSP thread_sp = m_process.GetThreadList().FindThreadByID(m_tid);
-  m_thread = thread_sp.get();
-  return *m_thread;
+  m_thread_sp = m_process.GetThreadList().FindThreadByID(m_tid);
+  return *m_thread_sp;
 }
 
 bool ThreadPlan::PlanExplainsStop(Event *event_ptr) {
@@ -133,11 +132,7 @@
           StateAsCString(resume_state), StopOthers());
     }
   }
-  bool success = DoWillResume(resume_state, current_plan);
-  m_thread = nullptr; // We don't cache the thread pointer over resumes.  This
-                      // Thread might go away, and another Thread represent
-                      // the same underlying object on a later stop.
-  return success;
+  return DoWillResume(resume_state, current_plan);
 }
 
 lldb::user_id_t ThreadPlan::GetNextID() {
Index: lldb/include/lldb/Target/ThreadPlan.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlan.h
+++ lldb/include/lldb/Target/ThreadPlan.h
@@ -600,9 +600,8 @@
   // For ThreadPlan only
   static lldb::user_id_t GetNextID();
 
-  Thread *m_thread; // Stores a cached value of the thread, which is set to
-                    // nullptr when the thread resumes.  Don't use this anywhere
-                    // but ThreadPlan::GetThread().
+  lldb::ThreadSP m_thread_sp; // Stores a cached value of the thread.  Don't use
+                              // use this anywhere but ThreadPlan::GetThread().
   ThreadPlanKind m_kind;
   std::string m_name;
   std::recursive_mutex m_plan_complete_mutex;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86388.287139.patch
Type: text/x-patch
Size: 4016 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20200822/9774f4bd/attachment.bin>