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

Preserve the original order of the sections #203

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

Conversation

batuzovk
Copy link
Contributor

COFF symbol table references sections by their index. We need to present them to the users in the original order, or they won't be able to use symbols from the COFF table correctly.

COFF symbol table references sections by their index. We need
to present them to the users in the original order, or they
won't be able to use symbols from the COFF table correctly.
@batuzovk batuzovk requested a review from woodruffw as a code owner February 25, 2025 11:52
@batuzovk
Copy link
Contributor Author

ntdll.dll.gz is an example of where the initial implementation fails. This .dll comes from wine32 distribution. It uses COFF symbols (and those can be encountered in PE files despite what MS documentation suggests), and the ordering of sections is different from the original file. For example, in dump-pe output .bss comes first, where in the original file .text is the first section. Consequently, a lot of symbols with section number 1 will be attributed to .bss instead of .text.

I looked through the source, and the sort seem to be redundant here. getSecForVA uses linear search, and no other function works with the vector directly.

@woodruffw
Copy link
Member

Hmm, good catch -- I added this back in #128 to make an implementation of Authenticode easier, but I can see why this would cause problems.

This would be a major behavioral change, so we'd probably need to do it with a 3.x release. But that's OK IMO.

I'll give this a more in-depth review later today 🙂

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.

2 participants