Skip to content

[lldb] Fix another race condition in Target::GetExecutableModule #145991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 26, 2025

Conversation

JDevlieghere
Copy link
Member

c72c0b2 fixed a race condition in Target::GetExecutableModule. The patch originally added the lock_guard but I suggested using the locking ModuleList::Modules() helper instead. That didn't consider that the fallback would still access the ModuleList without holding the lock. This patch fixes the remaining issue.

c72c0b2 fixed a race condition in Target::GetExecutableModule. The
patch originally added the lock_guard but I suggested using the locking
ModuleList::Modules() helper instead. That didn't consider that the
fallback would still access the ModuleList without holding the lock.
This patch fixes the remaining issue.
@JDevlieghere
Copy link
Member Author

CC @nd

@llvmbot
Copy link
Member

llvmbot commented Jun 26, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

c72c0b2 fixed a race condition in Target::GetExecutableModule. The patch originally added the lock_guard but I suggested using the locking ModuleList::Modules() helper instead. That didn't consider that the fallback would still access the ModuleList without holding the lock. This patch fixes the remaining issue.


Full diff: https://github.com/llvm/llvm-project/pull/145991.diff

1 Files Affected:

  • (modified) lldb/source/Target/Target.cpp (+6-3)
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 45a9e1196a049..a1556d892fd43 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1510,15 +1510,18 @@ bool Target::IgnoreWatchpointByID(lldb::watch_id_t watch_id,
 }
 
 ModuleSP Target::GetExecutableModule() {
-  // search for the first executable in the module list
-  for (ModuleSP module_sp : m_images.Modules()) {
+  std::lock_guard<std::recursive_mutex> lock(m_images.GetMutex());
+
+  // Search for the first executable in the module list.
+  for (ModuleSP module_sp : m_images.ModulesNoLocking()) {
     lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
     if (obj == nullptr)
       continue;
     if (obj->GetType() == ObjectFile::Type::eTypeExecutable)
       return module_sp;
   }
-  // as fall back return the first module loaded
+
+  // If there is none, fall back return the first module loaded.
   return m_images.GetModuleAtIndex(0);
 }
 

Copy link
Collaborator

@jasonmolenda jasonmolenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JDevlieghere JDevlieghere merged commit 76f3cc9 into llvm:main Jun 26, 2025
9 checks passed
@JDevlieghere JDevlieghere deleted the GetExecutableModuleRacePart2 branch June 26, 2025 23:44
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Jun 26, 2025
…m#145991)

c72c0b2 fixed a race condition in Target::GetExecutableModule. The
patch originally added the lock_guard but I suggested using the locking
ModuleList::Modules() helper instead. That didn't consider that the
fallback would still access the ModuleList without holding the lock.
This patch fixes the remaining issue.

(cherry picked from commit 76f3cc9)
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…m#145991)

c72c0b2 fixed a race condition in Target::GetExecutableModule. The
patch originally added the lock_guard but I suggested using the locking
ModuleList::Modules() helper instead. That didn't consider that the
fallback would still access the ModuleList without holding the lock.
This patch fixes the remaining issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants