Skip to content
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

IndirectAnalyzer: fix potential segfault #1672

Merged
merged 1 commit into from Feb 19, 2024

Conversation

ilil01
Copy link
Contributor

@ilil01 ilil01 commented Jan 11, 2024

When processing a call instruction, the IndirectControlFlowAnalyzer::FindAllThunks method assumes that both the instruction and its target are located in the same Region. This is not always the case — for example, in 32-bit libc there are instructions in __libc_freeres_fn section calling functions from .text. In such cases this method provokes a segmentation fault since the result of the getPtrToInstruction method is not tested against nullptr.

@bbiiggppiigg
Copy link
Collaborator

If the fix is just adding a if(target) check around the decoder and the for loop, it looks reasonable to me.
However, the indentation seems to be a bit off when viewed in the browser.

@ilil01
Copy link
Contributor Author

ilil01 commented Feb 8, 2024

If the fix is just adding a if(target) check around the decoder and the for loop, it looks reasonable to me. However, the indentation seems to be a bit off when viewed in the browser.

Yes, the fix is exactly that. What about the indentation, it's inconsistent in this whole function, especially due to mixed use of tabs and spaces. I decided to also make the indentation more consistent, at least in the changed scope. I can revert
indentation changes if that is more desirable.

@kupsch
Copy link
Contributor

kupsch commented Feb 13, 2024

@ilil01, can you open a bug report issue for this problem with the information on how to reproduce? I am unable to reproduce this with a simple test. This will also allow us to make a more robust fix in the future.

@ilil01
Copy link
Contributor Author

ilil01 commented Feb 14, 2024

@ilil01, can you open a bug report issue for this problem with the information on how to reproduce? I am unable to reproduce this with a simple test. This will also allow us to make a more robust fix in the future.

I've opened issue #1680

@kupsch
Copy link
Contributor

kupsch commented Feb 19, 2024

This prevents the crash. Fixing issue #1680, will be a more complete fix to find the correct region and instruction.

@kupsch kupsch merged commit 7f4708f into dyninst:master Feb 19, 2024
13 checks passed
@kupsch kupsch added parseAPI This issue is directly related to parseAPI user-reported This issue was reported by someone outside of the core Dyninst developers labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parseAPI This issue is directly related to parseAPI user-reported This issue was reported by someone outside of the core Dyninst developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants