fix(pe): only map file-backed sections to offsets#527
Conversation
supervacuus
changed the title
|
@supervacuus just so I fully understand, the other PR that adds permissive parsing to the source directories that I've just merged (and caused a merge conflict with the similar changes in this PR), does this fix the underlying perf issue you were seeing, or does it require the RVA fixes as well? |
No, they are independent. Both PRs can make sense independently. But I proposed the other PR due to performance issues this PR would resolve. It might still make sense to turn off imports parsing entirely if this PR lands, but having the option to turn it off would be less relevant for the downstream use cases in |
|
Oh, I am sorry... I thought you meant #522, but you were talking about #526, right? No, while #526 also adds the |
m4b
left a comment
m4b
left a comment
There was a problem hiding this comment.
LGTM, thanks for your patience! I'd like @kkent030315 to review if possible the pe::utils stuff but seems ready to go to me otherwise
There was a problem hiding this comment.
@kkent030315 could you review changes in this file? I generally defer to you on PE stuff :)
|
|
||
| if virtual_size == 0 { | ||
| read_size | ||
| } else if read_size == 0 { |
There was a problem hiding this comment.
The only question I have is regressions in other cases that might happen because this check was removed; to be fair, I can't remember why it was added, and there's no comment explaining the logic here, so if this fixes a big bug for you, and all our tests pass, I'm biased towards just merging.
There was a problem hiding this comment.
The actual fix is the early exit on size_of_raw_data == 0, which prevents this fallback to virtual_size. I see no other way in which read_size could be 0 now. That is why I removed the check, because it tests a case that should not exist once we exclude size_of_raw_data == 0. But maybe I misunderstood.
There was a problem hiding this comment.
This check was added in #396. It was added to fix parsing of the file in https://github.com/ideeockus/goblin/blob/f504af9de4c86bdf697ceb6876f2683c0226886f/tests/bins/pe/exception_rva_mapping/amdcleanuputility.exe.upx_packed, which was giving the same error as #307. That error reappears with this PR regardless of whether this check is removed or not (the earlier early return is enough to cause it). I think the change in this PR is fine, and #396 should be fixed differently (use the permissive parsing option).
supervacuus
force-pushed
the
fix/pe-file-backed-rva
branch
from
59932f9 to
f4b2c70
Compare
|
Did you find a conclusion regarding this change and #522? |
|
@supervacuus we can merge this if it doesn't regress anything and fixes something for you. |
I am fine with either solution, since in the downstream uses, we don't need the imports (yet). So having an option to turn off the import parser is sufficient. I see it like this: the import parser option is the safer bet in terms of expected regressions, but also a much coarser solution. However, it might also make sense for uses other than the concrete performance hit we see downstream. This PR otoh, resolves the root cause of the actual performance issue and should likely land independently of the option from #522. As @philipc raised in #527 (comment), we at least know of one regression that can be rectified by the permissive parsing mode introduced since the fix. |
|
ok thanks for the summary; it sounds like for your usecases you'll probably use the options introduced in 522, but this is also a nice perf fix if someone uses the defaults (which is to parse the perf table). It sounds like there isn't any danger to merge this in terms of regressions of default parser, so let's merge it; if we encounter something we can always revert since it's backwards compatible. Thank you for your patience! |
|
NB: backwards compatible |
|
released in 0.10..6 |
3 participants
This is complementary to the
parse_importsoption change (#522). That change adds an escape hatch for callers who do not need import parsing. This PR otoh, solves the underlying issue that prompted this option (which could still make sense). The result is that, even with import parsing enabled in permissive mode, we get the same baseline runtime as if import parsing were disabled.For on-disk PE parsing, sections with
SizeOfRawData == 0are virtual-only.goblincurrently still maps RVAs in such sections by falling back toVirtualSize. On affected files (in our case,mingw/msysdebug companions), this causes unrelated bytes to be parsed as imports, and similarly affects other PE data directories.This PR makes PE RVA resolution treat sections with
SizeOfRawData == 0as not file-backed. Resource parsing is also made permissive-mode-friendly for the same class of virtual-only.rsrcdirectories, and themalformed_resource_tree()test is adjusted to assert parse failure rather than a specific panic path since the corrected mapping changes the failure point.