[
https://issues.apache.org/jira/browse/TIKA-4936?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18122113#comment-18122113
]
ASF GitHub Bot commented on TIKA-4936:
--------------------------------------
tballison commented on PR #3263:
URL: https://github.com/apache/tika/pull/3263#issuecomment-5961774884
From my bot:
```
Edge cases
1. The amplification fix can be bypassed, and output is unbounded across
groups (PEIconExtractor.java:368-380, emitIcons). The fix deduplicates by icon
id, but 256 distinct ids can all point at the same
data entry. Separately, nothing limits how many groups are emitted or
how many bytes they total, so ~10k groups can share one group record and each
rebuild a 64 MB .ico.
- Confirmed by PoC: a 214 KB EXE with 40 groups emitted 2.0 GB (9585×).
Within the 20k-entry budget that scales to about 600 GB from a ~0.5 MB file,
which gets written to disk under -z or /unpack.
- The commit message says it "closes a 256x amplification"; that claim
is false.
- Fix: deduplicate images by (offset, size), not id. Also add a
total-emitted-bytes budget across all groups, a small multiple of the section
length. A byte cap alone breaks legitimate
language-variant groups that share images, which is why the multiple
is needed.
- The PoC is ready to become the regression test:
~/Desktop/claude-todo/pr3263/PocAmplificationTest.java.
2. The "lazy" buffer still allocates from header-declared sizes
(Section.ensure, :443-447). The buffer is sized to end before any read. A tiny
file that declares a 64 MB raw section and has one data
entry near its end forces a 64 MB allocation. The amount is capped, so
this is low severity, but it contradicts the commit's "grows with what is
actually read". Grow in chunks bounded by bytes
actually read.
Mechanical fixes
- @deprecated since 4.1.1 should say 4.2.0: main is 4.2.0-SNAPSHOT. Keep
4.1.1 only if a backport is planned.
- CHANGES.txt now has a 4.2.0 section, so the PR's "no unreleased section"
reason no longer holds. Add an entry.
Your decision
- Default-on. Upgraders will see every EXE/DLL suddenly produce embedded
documents (extra RMETA rows and extra unpacked files). I'd keep it on, but say
so in CHANGES.
- Config shape. extractIcons is a plain bean setter. JSON config reaches
it, but there's no per-request (ParseContext) override and no test that config
actually reaches it. The 4.x pattern is a Config
class plus a JsonConfig constructor (see PSDParser). Fine to defer.
Hygiene
- The binary fixtures (3 MinGW-built PEs and 2 .ico files) come with no
source or build recipe. Ask for the .rc/.c files and the build command in a
test comment so they can be reproduced and checked.
```
Let me know what you think.
> Extract icons from PE executables (EXE/DLL) as embedded documents
> -----------------------------------------------------------------
>
> Key: TIKA-4936
> URL: https://issues.apache.org/jira/browse/TIKA-4936
> Project: Tika
> Issue Type: New Feature
> Environment:
>
>
>
> Reporter: Dominik Schmidt
> Priority: Major
>
> h3. Background
> {{ExecutableParser}} currently only reads the COFF file header of PE files
> (EXE/DLL) and emits basic metadata (machine type, architecture bits,
> endianness, created date). The resource section ({{.rsrc}}) is not parsed, so
> resources such as the application icon are not accessible via Tika.
> h3. Proposal
> Parse the PE resource directory and emit each icon group as an embedded
> document:
> * Parse optional header, data directories and section table to locate the
> resource directory (RVA → file offset).
> * Walk the resource tree (type → name/ID → language).
> * For each {{RT_GROUP_ICON}} (type 14), reconstruct a standalone {{.ico}}
> file from the {{GRPICONDIR}} and the referenced {{RT_ICON}} (type 3) entries:
> write an {{ICONDIR}} header and replace the 2-byte resource IDs ({{nID}})
> with 4-byte image offsets.
> * Pass each reconstructed file to the {{EmbeddedDocumentExtractor}} with:
> ** {{Content-Type}}: {{image/vnd.microsoft.icon}}
> ** {{resourceName}}: e.g. {{icon_<id-or-name>.ico}}
> ** {{embeddedResourceType}}: {{THUMBNAIL}} for the first icon group (the icon
> shown by Windows Explorer), {{ATTACHMENT}} for all others
> ** the resource language ID
> Single {{RT_ICON}} entries are intentionally not emitted on their own:
> BMP-based entries are not valid standalone images (no {{BITMAPFILEHEADER}},
> double height for the AND mask), and emitting them would duplicate the group
> data.
> h3. Robustness
> The parser must handle malformed or malicious binaries gracefully:
> * bound the resource tree depth (normally 3 levels) and the number of entries
> * validate all offsets and sizes against the file size
> * guard against cycles in the resource directory
> * failures while extracting resources must not break the existing metadata
> extraction
> h3. Out of scope (possible follow-ups)
> * {{RT_GROUP_CURSOR}}/{{RT_CURSOR}} → {{.cur}}
> * {{RT_MANIFEST}} as an embedded XML document
> * {{VS_VERSIONINFO}} (product name, file version, company) as metadata
> h3. Acceptance criteria
> * Icons of 32- and 64-bit EXE and DLL test files are extracted as valid
> {{.ico}} files that are detected as {{image/vnd.microsoft.icon}}.
> * Icon groups containing both PNG- and BMP-encoded entries are supported.
> * Files without a resource section, or without icons, are parsed as before,
> with no embedded documents.
> * Truncated or corrupted resource sections do not throw and still yield the
> existing metadata.
> * Unit tests use small, license-compatible test files.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
