-
Notifications
You must be signed in to change notification settings - Fork 540
Small refactor of is_valid_function_name #440
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
j-t-1
wants to merge
4
commits into
erocarrera:master
Choose a base branch
from
j-t-1:is_valid_function_name
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be curious to know where these lists of characters originally came from. I'm not seeing many good references for the allowed set of characters in mangled function names, other than looking at the name mangling functions implemented in compilers and figuring out what set of characters can appear in them based on their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first set are mangled / decorated characters. The dollar sign, question mark, @ sign and underscore are used by the Microsoft linker.
The less-than sign and greater-than sign were empirically added in issue #61.
I do not know where the parentheses and period are from, are they decoration from other compilers?
IDA has support for demangling names; @skochinsky would you be able to help us?
Does
b"._?@$()<>"correspond to mangled characters in PE files?We use this in functions
parse_export_directoryandparse_imports.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j-t-1
From
ida.cfg:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. the PE file format does not impose any limitation on the characters used, and
GetProcAddressshould in theory work with any null-terminated byte string. So, I would suggest to not perform any explicit validation, just maybe invalid character replacement on output.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skochinsky thank you.
I have updated this PR to include more characters, referencing
ida.cfg(hope okay).Changing the code to not perform explicit validation is a useful suggestion.
pefile/pefile.py
Lines 2316 to 2318 in 4b3b1e2
This function is used by
parse_export_directory(uses the extra characters) andparse_imports(does not use the extra characters):pefile/pefile.py
Lines 5515 to 5517 in 4b3b1e2
pefile/pefile.py
Lines 6064 to 6065 in 4b3b1e2
pefile is using this for imports that are difficult to parse:
pefile/pefile.py
Lines 6091 to 6103 in 4b3b1e2
So pefile could be modified to handle this kind of case differently.
I will keep this PR as is, but something to consider @erocarrera and @nightlark.