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

Fixed #267 #268

Merged
merged 2 commits into from Apr 11, 2021
Merged

Fixed #267 #268

merged 2 commits into from Apr 11, 2021

Conversation

2vg
Copy link
Contributor

@2vg 2vg commented Mar 29, 2021

related: #267

The problem here was solved by using parse_with_otps.
I had some time, so I debugged and confirmed that the problematic code I presented didn't cause any problems.
No tests were added because I didn't have time to write the tests.
If you have time, please help me :<

@m4b
Copy link
Owner

m4b commented Apr 5, 2021

@2vg thank you for the patch! LGTM, looks like rustfmt failed, can you run it on that file? thanks, once CI passes can merge

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.

just needs rustfmt; if you don't want me to squash, you can just ammend and force push the rustfmt change if you prefer

@@ -227,11 +227,12 @@ impl<'a> SyntheticImportDirectoryEntry<'a> {
opts,
) {
debug!("Synthesizing lookup table imports for {} lib, with import lookup table rva: {:#x}", name, import_lookup_table_rva);
let import_lookup_table = SyntheticImportLookupTableEntry::parse::<T>(
let import_lookup_table = SyntheticImportLookupTableEntry::parse_with_opts::<T>(
Copy link
Owner

Choose a reason for hiding this comment

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

@ko1N since you implemented this feature, does this look correct to you?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know why not passing the opts along would hurt, and seems to resolve @2vg 's issue (though I confess I'm not sure what was the exact problem), so I assume this is ok :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the table entry will not be resolved properly when not passing the opts in case rva_resolve is disabled.

Since we left all regular parse functions untouched (as in the regular/"old" behavior) this was an oversight on my part. Luckily those changes could've only caused trouble when using mapped binary dumps. Either way, the change look totally fine to me.

@m4b m4b merged commit bad24ab into m4b:master Apr 11, 2021
@m4b
Copy link
Owner

m4b commented Apr 11, 2021

@2vg thank you for your patience, merged; would you like a new release? I will likely add this to the 0.4 release, which has one (minor) breaking change, but which should not affect your usecase at all (PE, iiuc)

@m4b
Copy link
Owner

m4b commented Apr 11, 2021

Ok 0.4.0 is out :)

@2vg 2vg deleted the fix/267 branch April 16, 2021 05:23
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.

None yet

3 participants