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

Change the section name parsing to only remove trailing zeroes #979

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

HoundThe
Copy link
Member

Fixes #958

The removal of all NULL chars in the pelib was intentional as in the comment:

	// Historically, PELIB copies the name of the section WITHOUT zero chars,
	// even if the zero chars are in the middle. Aka ".text\0\0X" results in ".textX"

Not sure what was the reason, but based on the issue, I've changed the parsing to strip only the trailing zeroes.

@PeterMatula PeterMatula self-requested a review July 16, 2021 12:55
Copy link
Collaborator

@PeterMatula PeterMatula left a comment

Choose a reason for hiding this comment

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

These tricky iterations again :-D
Corner cases:

  1. I think it can handle even the case when an entire name is \0. But do we have this in regression tests? I think not. Can we add it? The easiest would probably be to hack some sample, possible the one already used if there are some more sections to play with, with some hex editor.
  2. Can we handle if the name is not terminated at all - all the chars are printable? I think we decrease end once anyway, and then assign end + 1 as end. So it is probably ok. But again, can we have a test for this - if there already isn't one?

@HoundThe
Copy link
Member Author

I think I tried all of these corner cases and I still have the samples I've created for this. I'll have a look into the regression suite if there is something like that, if not then I'll create a single sample that has these cases and update the tests.

@HoundThe
Copy link
Member Author

I've added the test cases

@PeterMatula PeterMatula merged commit 3d506f2 into avast:master Jul 20, 2021
PeterMatula added a commit that referenced this pull request Jul 20, 2021
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.

Improve PE section name handling of escaped sequences
2 participants