[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'#188377
Merged
Conversation
Member
|
@llvm/pr-subscribers-lldb Author: GeorgeHuyubo ChangesContext: When Fix this by adding After this fix, lldb is not crashing anymore, new symbols from a symbol file are correctly loaded Full diff: https://github.com/llvm/llvm-project/pull/188377.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index fa4483c93e639..1b0dc7d26ba5b 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -457,6 +457,9 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
/// Redraw the statusline if enabled.
void RedrawStatusline(std::optional<ExecutionContextRef> exe_ctx_ref);
+ /// Flush cached state (e.g. stale execution context in the statusline).
+ void Flush();
+
/// This is the correct way to query the state of Interruption.
/// If you are on the RunCommandInterpreter thread, it will check the
/// command interpreter state, and if it is on another thread it will
diff --git a/lldb/include/lldb/Core/Statusline.h b/lldb/include/lldb/Core/Statusline.h
index a5ab1927b57f5..26bf9323d5650 100644
--- a/lldb/include/lldb/Core/Statusline.h
+++ b/lldb/include/lldb/Core/Statusline.h
@@ -32,6 +32,9 @@ class Statusline {
/// Redraw the statusline.
void Redraw(std::optional<ExecutionContextRef> exe_ctx_ref);
+ /// Clear the cached execution context to discard stale pointers.
+ void Flush();
+
/// Inform the statusline that the terminal dimensions have changed.
void TerminalSizeChanged();
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index e1b2ce1b063e0..0c449b01da040 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1284,6 +1284,15 @@ void Debugger::RedrawStatusline(
m_statusline->Redraw(exe_ctx_ref);
}
+void Debugger::Flush() {
+ std::lock_guard<std::mutex> guard(m_statusline_mutex);
+
+ if (!m_statusline)
+ return;
+
+ m_statusline->Flush();
+}
+
ExecutionContext Debugger::GetSelectedExecutionContext() {
bool adopt_selected = true;
ExecutionContextRef exe_ctx_ref(GetSelectedTarget().get(), adopt_selected);
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index bdc649580637c..357ecbc825200 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -125,6 +125,8 @@ void Statusline::UpdateScrollWindow(ScrollWindowMode mode) {
m_debugger.RefreshIOHandler();
}
+void Statusline::Flush() { m_exe_ctx_ref.ClearFrame(); }
+
void Statusline::Redraw(std::optional<ExecutionContextRef> exe_ctx_ref) {
// Update the cached execution context.
if (exe_ctx_ref)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 587d9f6408ae4..b9adc90cde436 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6025,6 +6025,7 @@ void Process::Flush() {
m_extended_thread_stop_id = 0;
m_queue_list.Clear();
m_queue_list_stop_id = 0;
+ GetTarget().GetDebugger().Flush();
}
lldb::addr_t Process::GetCodeAddressMask() {
|
clayborg
reviewed
Mar 26, 2026
clayborg
left a comment
clayborg
left a comment
Contributor
There was a problem hiding this comment.
@JDevlieghere we ran into the status line crashing us when we added symbols to an ELF file that was stopped in a __lldb_unamed_symbolXXX symbol. It is hard to reproduce in a test case as it requires a specific order or things happening to the heap.
The issue is the StackID cached the Symbol * in StackID.m_symbol_scope. Adding symbols can cause symbols to get added to the symbol table when the new symbol file is loaded which contains local symbols. This causes the symbol table vector to be re-sized and invalidates the pointer. But the status line now has a stale SymbolContextScope *m_symbol_scope cached which can cause a crash depending on if the memory that this pointed to points to other random data.
We need a way to invalidate any out of date SymbolContextScope * entries that anyone is holding onto. In this case the StatusLine, but we might need to also clear other things within the debugger in the future. Like maybe the ExecutionContext in the command interpreter.
Member
|
This one is particularly scary because LLDB passes out I think we need to rethink our Symbol management entirely if we're allowed to add to the symbol tables whenever we want. |
Contributor
Author
|
@JDevlieghere Renamed function to be more specific, we can always rename it later once it clear more things |
clayborg
reviewed
Apr 9, 2026
clayborg
left a comment
clayborg
left a comment
Contributor
There was a problem hiding this comment.
@JDevlieghere are you ok with the current state here? This is a stop gap fix to stop things from crashing.
The much harder but root cause fix is to have the symbol table be able to have multiple vectors of symbols and have APIs to add symbols by adding a complete vector. That way when an object file is parsing its symbol table (original executable), it will create its own vector and add the vector into the symbol table, and then when another object file (dSYM or newly added symbol file) can do the same thing and register another vector of symbols. This will ensure that no symbol pointers can go stale. This change will be much larger and more complex, but should be done at some point.
Member
|
@GeorgeHuyubo This PR was merged without approval. Please review our Code Review Policy:
I still have two concerns with this patch even if we want to go for a pragmatic solution:
If this can be addressed quickly I'm fine with fixing forward, but otherwise please revert while we iterate on a solution. |
Contributor
Author
|
Added test: #193854 @JDevlieghere |
GeorgeHuyubo
added a commit
that referenced
this pull request
Apr 24, 2026
…ne (#193854) Add a test that exercises the code path fixed in [88f024223cc4](#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
llvm-sync Bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 24, 2026
…in statusline (#193854) Add a test that exercises the code path fixed in [88f024223cc4](llvm/llvm-project#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
llvm-upstreamsync Bot
pushed a commit
to qualcomm/cpullvm-toolchain
that referenced
this pull request
Apr 24, 2026
…in statusline (#193854) Add a test that exercises the code path fixed in [88f024223cc4](llvm/llvm-project#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
cpullvm-upstream-sync Bot
pushed a commit
to navaneethshan/cpullvm-toolchain-1
that referenced
this pull request
Apr 24, 2026
…in statusline (#193854) Add a test that exercises the code path fixed in [88f024223cc4](llvm/llvm-project#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
yingopq
pushed a commit
to yingopq/llvm-project
that referenced
this pull request
Apr 29, 2026
…ne (llvm#193854) Add a test that exercises the code path fixed in [88f024223cc4](llvm#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
KHicketts
pushed a commit
to KHicketts/llvm-project
that referenced
this pull request
Apr 30, 2026
…ne (llvm#193854) Add a test that exercises the code path fixed in [88f024223cc4](llvm#188377) ("[lldb] Fix stale Symbol pointer crash in statusline after 'target symbols add'"). The bug: when `target symbols add` is called, `Symtab::AddSymbol()` can reallocate the underlying `std::vector<Symbol>`, invalidating all existing `Symbol*` pointers. The statusline caches an `ExecutionContextRef` containing a `StackID` with a `SymbolContextScope*` (which can be a `Symbol*`). If a concurrent statusline redraw occurs between the Symtab reallocation and `Process::Flush()` (e.g. from a progress event on the event handler thread), the cached `StackID` matches the old frame via pointer-equal comparison, and `GetSymbolContext()` dereferences the dangling `Symbol*`. The test: 1. Builds a stripped binary and its unstripped counterpart. 2. Runs the stripped binary and stops at a breakpoint in `main`. 3. Enables the statusline (populating the cached `ExecutionContextRef`). 4. Calls `target symbols add` to add debug symbols back. 5. Forces a statusline redraw via terminal resize, which calls `Redraw(std::nullopt)` using the cached (potentially stale) execution context. 6. Verifies the statusline still renders correctly. The dangling pointer is a use-after-free that silently succeeds in non-sanitizer builds (the freed memory is still accessible). This test is most effective under AddressSanitizer, which poisons freed memory and immediately detects the stale access. Co-authored-by: George Hu <georgehuyubo@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context:
lldb might crash when running to a debuggee crashing state and do a target symbols add command.
Backtrace:
When
target symbols addis run,Symtab::AddSymbol()can reallocate the underlyingstd::vector<Symbol>and resize it, invalidating all existing Symbol* pointers. WhileProcess::Flush()clears stale stack frames, the statusline caches its ownExecutionContextRefcontaining aStackIDwith aSymbolContextScope*(which can be aSymbol*). This cached reference is not cleared byProcess::Flush(), so the next statusline redraw accesses a dangling pointer and crashes.Fix this by adding
Statusline::Flush()which clears the cached frame,Debugger::Flush()which forwards to it under the statusline mutex, and callingDebugger::Flush()fromProcess::Flush()so that all flush paths (symbol add, exec, module load) also invalidate the statusline's stale state.After this fix, lldb is not crashing anymore, new symbols from a symbol file are correctly loaded