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

pe: align section reads to FileAlignment, pad sections to SectionAlignment #436

Merged
merged 9 commits into from Aug 19, 2021

Conversation

williballenthin
Copy link
Contributor

closes #435

@atlas0fd00m
Copy link
Contributor

hey @williballenthin ,

can you help me understand here? the unittests seem to indicate pretty significant failures that don't immediately appear to be "more accurate." i could be mistaken.

thanks!
@

@atlas0fd00m
Copy link
Contributor

specifically, this PR is breaking analysis of Firefox.exe in ways that it should not. any idea why?

@williballenthin
Copy link
Contributor Author

sure, let me dig into the test failures. i'm glad test cases caught a potential regression here.

we're in the realm of "what happens if sizes (with and without alignment) are pretty weird?" so I'm not super confident in the intuition/reasoning behind this PR. so, i'll also do some bulk runs against a moderate number of PE files and see what happens.

@williballenthin
Copy link
Contributor Author

sidebar issue: i cannot see the CI results (without logging into CircleCI which requires giving access to my private repos, which i object to on principle). maybe another reason to consider GH Actions?

vivisect/parsers/pe.py Outdated Show resolved Hide resolved
tweak alignment calculation
@atlas0fd00m
Copy link
Contributor

ahhh. these failures look much better.
i'd say it's probably safe to go through https://app.circleci.com/pipelines/github/vivisect/vivisect/342/workflows/aa66fd4a-6c8c-4fee-bb83-f49cc9e446e1/jobs/372 and modify the tests to match what we're actually getting now. these failures indicate an increased size, not a loss of analysis/locations.

the increased Undefined answer is just a sad artifact of adding padding to the workspace, and likely doesn't indicate any regression.

make those few changes and the tests should pass. if you do it quickly, i'll merge it in before 4:30pm EDT. otherwise, @rakuy0 will have to do it since i'll be gone.

@williballenthin

This comment has been minimized.

@williballenthin

This comment has been minimized.

@williballenthin

This comment has been minimized.

@williballenthin
Copy link
Contributor Author

ok no unexpected failures when i run these tests locally. should be good to go.

@atlas0fd00m atlas0fd00m merged commit 5ac4e55 into vivisect:master Aug 19, 2021
@williballenthin williballenthin deleted the fix-435 branch August 19, 2021 21:54
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.

viv fails to read strings at the end of some sections
2 participants