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

fix: Iterate all ImageDebugDirectory entries #319

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

Conversation

Swatinem
Copy link
Contributor

Turns out, a PE file can have more than one ImageDebugDirectory.
Thus far, goblin only looks at the very first one, trying to interpret
it as a CV record.

This code changes that logic to rather iterate over all the entries to
find the one that is a CV record.
We still only capture a single entry in the DebugData for backwards
compatibility reasons. In the future we could as well capture all of
them.

Turns out, a PE file can have more than one ImageDebugDirectory.
Thus far, goblin only looks at the very first one, trying to interpret
it as a CV record.

This code changes that logic to rather iterate over all the entries to
find the one that is a CV record.
We still only capture a single entry in the `DebugData` for backwards
compatibility reasons. In the future we could as well capture all of
them.
@Swatinem
Copy link
Contributor Author

fun fact: I wrote a detailed blog post about how I made this discovery: https://swatinem.de/blog/format-ossification/

@m4b
Copy link
Owner

m4b commented Jul 30, 2022

looks like a CI failure?

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

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

Let's fix the CI failure then I'll do another pass at review, and thanks for this!

Ok(DebugData {
image_debug_directory,
codeview_pdb70_debug_info,
image_debug_directory: entries.pop().unwrap(),
Copy link
Owner

Choose a reason for hiding this comment

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

can this unwrap ever fail?

.collect::<Result<Vec<ImageDebugDirectory>, _>>()?;

// find the debug directory that references the codeview record
for (idx, idd) in entries.iter().enumerate() {
Copy link

Choose a reason for hiding this comment

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

I know the original code just tries parsing CodeviewPDB70DebugInfo directly, but you should additionally be able to look at the data_type field to see if it is IMAGE_DEBUG_TYPE_CODEVIEW per the MSDN docs.

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