feat(pe): add parse_imports option to ParseOptions#522
Conversation
|
Just wanted to ping if this kind of change is of interest. I have also now added a new PR (#527) that fixes the underlying issue. It might still make sense to be able to turn off import parsing, but if #527 is accepted, I could understand if you didn't want to add another parser option. The resulting runtimes are now essentially the same for either PR. |
|
I'm ok with this i think, @kkent030315 is this something you think is fine? |
| if opts.parse_imports { | ||
| if let Some(&import_table) = optional_header.data_directories.get_import_table() { |
There was a problem hiding this comment.
can we merge it to one?
| if opts.parse_imports { | |
| if let Some(&import_table) = optional_header.data_directories.get_import_table() { | |
| if opts.parse_imports && let Some(&import_table) = optional_header.data_directories.get_import_table() |
There was a problem hiding this comment.
rust is pinned at 1.85 in CI which complains about the let chain.
Otherwise fixed in: 9f632ae
There was a problem hiding this comment.
We can use
if let (true, Some(&import_table)) = (
opts.parse_imports,
optional_header.data_directories.get_import_table(),
) {
}if you prefer it to if-nesting. However, that means get_import_table() will be called unconditionally (which is not a costly invocation, but a change in behavior nonetheless).
Changed here: db951f2
|
I'm sorry @supervacuus there's a merge conflict with another pr that touched options, would you mind to rebase? I'll merge it asap as I want to cut a release tonight or tomorrow at latest. If don't hear I'll manually resolve it in editor though I'd prefer not |
supervacuus
force-pushed
the
feat/add_parse_imports_pe_option
branch
from
db951f2 to
b6d0a11
Compare
supervacuus
force-pushed
the
feat/add_parse_imports_pe_option
branch
from
b6d0a11 to
0efa02f
Compare
I rebased and squashed my commits in 0efa02f. |
|
NB: backwards compatible |
|
released in 0.10.6 |
…964) This is a follow-up to #960 and depends on an upstream change in `goblin` that has not yet been accepted (m4b/goblin#522). It makes the integration of PE+DWARF debug companions into the DIF pipeline practical (see getsentry/sentry-cli#3240) --------- Co-authored-by: David Herberth <[email protected]>
### Description Extracted from #3237 Some tests currently fail, because `symbolic` updated its `zip` dependency (`2.4.2` to `7.2.0`) since the last bump. This changed the encoding and invalidated a bunch of test assertions. A fix proposal for the failing tests is #3239. This change was triggered by bumping `symbolic` to `12.17.3` to include getsentry/symbolic#960. The new permissive PE parser fixes in getsentry/symbolic#960 exposed performance edge cases for some of the debug companions during PE import-table parsing, the result of which neither `symbolic` nor `sentry-cli` really needs. The PR addresses the issue in the following way: I introduced a new `goblin` option that excludes the `PE` import table from parsing. Of course, this requires further upstream PRs (`cargo` points `goblin` and `symbolic` to dev branches) before we can merge this, but I wanted to check whether you are generally in agreement with these changes before going upstream. Upstream changes required: - m4b/goblin#522 - getsentry/symbolic#964 Ideally, this PR gets merged after #3238 and #3239, because both solve issues this PR is affected by. If we drop or defer #3239, we need to update all snapshots and magic digests here first. ### Issues At least partially fixes getsentry/sentry#104738 ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
3 participants
Parsing
PEimport tables is expensive: for each DLL, the parser does a full pass over the import lookup table (resolving every symbolRVAvia a linear scan over sections), a second pass over the import address table, and then a third reshaping pass inImport::parse. For binaries with large or malformed import tables, this adds up fast, especially when the caller only needs headers, debug info, or exports.This PR adds a
parse_importsflag toParseOptions(default:true) with awith_parse_importsbuilder method, allowing callers to skip import table parsing entirely. When set tofalse,import_data,imports, andlibrariesremain empty while everything else is parsed as usual.This is a follow-up to #506 (#508).