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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
