Skip to content

fix: avoid OOB access if source code unreadable #727

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcc
Copy link

@pcc pcc commented May 26, 2025

We set m_startLine and m_numLines according to the debug info but if the source file is unreadable, or if it is readable but does not have enough lines, this will lead to an OOB access and a crash in SourceCodeModel::data() -> HighlightedText::lineAt(). Fix it by making it so that we only set m_startLine and m_highlightedText once we have successfully read the file and ensure that they are in bounds of the lines array.

@pcc
Copy link
Author

pcc commented May 26, 2025

(I bet this is the real fix for #702.)

@milianw
Copy link
Member

milianw commented May 26, 2025

Thanks, can you tell me how to reproduce this? IIUC just make the source file unreadable or something like that, yes?

@GitMensch
Copy link
Contributor

According to the title - just change it to be four lines after compile (gdb will show the file and disassembly if requested, but give a note about "source file newer", if missing then it will show "source file not found" or similar, if I'm right).
As both cases are a bit different I suggest to include both options in the automated tests, if possible.

We set m_startLine and m_numLines according to the debug info but
if the source file is unreadable, or if it is readable but does not
have enough lines, this will lead to an OOB access and a crash in
SourceCodeModel::data() -> HighlightedText::lineAt(). Fix it by making
it so that we only set m_startLine and m_highlightedText once we have
successfully read the file and ensure that they are in bounds of the
lines array.
@pcc
Copy link
Author

pcc commented May 26, 2025

Thanks, can you tell me how to reproduce this? IIUC just make the source file unreadable or something like that, yes?

Right, or just delete the source file. I ran into this bug when analyzing a binary that had some prebuilt (by the distribution) object files with debug info statically linked into it, and I noticed while debugging the issue that truncated files would have the same problem.

Showing a warning in this case would be a good idea for a followup change. We should probably just fix the crash to begin with.

@@ -124,6 +122,9 @@ void SourceCodeModel::setDisassembly(const DisassemblyOutput& disassemblyOutput,
m_lines = sourceCode.split(QLatin1Char('\n'));

m_highlightedText.setText(m_lines);

m_startLine = std::min(m_lines.size(), minLineNumber - 1); // convert to index
Copy link
Member

Choose a reason for hiding this comment

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

in qt6 this would require casts but in general I wonder if we really need the min - can you double check?

Copy link
Author

@pcc pcc Jun 1, 2025

Choose a reason for hiding this comment

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

With this diff on top of this PR:

diff --git a/src/models/sourcecodemodel.cpp b/src/models/sourcecodemodel.cpp
index f7f7fcc..8aa04a8 100644
--- a/src/models/sourcecodemodel.cpp
+++ b/src/models/sourcecodemodel.cpp
@@ -123,8 +123,8 @@ void SourceCodeModel::setDisassembly(const DisassemblyOutput& disassemblyOutput,
 
     m_highlightedText.setText(m_lines);
 
-    m_startLine = std::min<int>(m_lines.size(), minLineNumber - 1); // convert to index
-    m_numLines = std::min<int>(m_lines.size() - m_startLine, maxLineNumber - minLineNumber + 1); // include minLineNumber
+    m_startLine = minLineNumber - 1; // convert to index
+    m_numLines = maxLineNumber - minLineNumber + 1; // include minLineNumber
 }
 

I can crash hotspot like this (same reproducer as KDAB/perfparser#42):

mv 1.c 1.c.bak
touch 1.c
~/src/hotspot-up/result/bin/hotspot perf.data

and then view the disassembly for main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants