tballison commented on PR #3037:
URL: https://github.com/apache/tika/pull/3037#issuecomment-5347083098
This is is really nice.
Claude has a few nits:
```
1. MEDIUM (compat/docs) — CHANGES undersells the behavior change every
upgrader inherits. With sub-class-of image/tiff added, a named
NEF/ARW/PEF/DNG that used to detect as image/tiff (TiffParser, metadata
only) now detects as image/x-raw-* and emits thumbnail-N.jpg
attachments in /rmeta and /unpack; CR2 keeps its detection but gains
attachments. Downstream consumers counting attachments or keying on
container Content-Type see changes CHANGES doesn't mention. One added
sentence fixes it.
2. MEDIUM (DoS lever, cheap fix) — byte-at-a-time unbuffered I/O. Every
value read bottoms out in single-byte RandomAccessFile.read() with
per-entry re-seeks (RawTiffParser.java:258-287, 408-427). A ~25KB
crafted file can force ~2.7×10⁸ syscalls (32 IFDs × 1024 entries ×
1024-count LONG8 values) — bounded but tens of seconds of CPU per tiny
file under concurrent load. Fix: read each IFD's entry table (≤20KB,
already bounds-checked) into one byte[] and parse from memory.
3. MEDIUM (design, maintainer call) — config ignored in ParseContext.
parse() reads only the constructor-bound config (:142,:297), never
context.get(RawTiffParserConfig.class, defaultConfig) — deviating from
the PDFParser convention. Trade-off is real: the security reviewer
counted "clients can't flip it per-request" as a hardening positive;
the correctness reviewer flags it as an API-freeze inconsistency a
ParseContext user silently loses to. Decide which contract you want
before 4.0 freezes it.
4. LOW/MEDIUM (scope-creep compat) — new II\x2b\x00 magic on image/tiff
routes plain little-endian BigTIFF .tif files to TiffParser, which
can't read BigTIFF → new parse exception where there was silence.
Extends a pre-existing gap (BE BigTIFF magic was already there), but
it's an unflagged detection change beyond the raw-file scope and absent
from CHANGES.
5. LOW — dedup is offset-only and untested (two reviewers independently).
A FFD8 FFD8... run defeats it via shifted offsets; real bound is
MAX_IFDS=32, so worst case ≈ 32× fileLength of downstream JPEG work —
tolerable, but the commit's "bound extraction work" claim overstates
it. An aggregate byte budget would make it true; either way, deleting
the dedup set today fails no test — add the two-IFD fixture test.
6. LOW — raw files silently lose the whole-file OCR branch.
RawTiffParser.parse fully replaces AbstractImageParser.parse, dropping
its OCR fallback for these types. Arguably better (previews get OCR'd
as JPEGs through the embedded pipeline), but undocumented and untested.
7. LOW — oversized previews are skipped with no signal (:296-297) — no
warning metadata, unlike structural failures which are recorded. Also:
CHANGES documents extractPreviews but not maxPreviewLengthBytes; the
"tens of MB is already generous" comment sits on a 100MB constant.
8. Test gaps: no hostile/truncated classic-TIFF test (the caps, cycle
guard, and exception-recording path at :153-157 are all unexercised);
no big-endian BigTIFF preview-extraction test (only LE fixture + BE
empty-header test). Nits: bare type codes 3/4/13/16/18 vs the file's
own named-constant convention; the
```
--
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]
