[
https://issues.apache.org/jira/browse/TIKA-4869?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110592#comment-18110592
]
ASF GitHub Bot commented on TIKA-4869:
--------------------------------------
tballison commented on PR #3115:
URL: https://github.com/apache/tika/pull/3115#issuecomment-5508249776
This is really nice.
My bot has maybe one or two things worth changing. The hygiene thing with
MediaType.parse should be fixed now in main(?).
Let me know what you think, and thank you!
```
1. edge-case — the offset gate accepts any recognized type, not a video;
that makes a JPEG recursive
MotionPhoto.java:222 — if (type == null ||
MediaType.OCTET_STREAM.equals(type)) { return; }
Two measured consequences:
- Replace the trailing 1583 bytes of testJPEG_MotionPhoto.jpg with ASCII
text → a second document is emitted, text/plain, named motion-photo.mp4.
CHANGES.txt and the PR description both say nothing is emitted "when the bytes
there are not a video". That is
not what the code does.
- outer || inner, where outer is the fixture with Item:Length rewritten to
|inner| and inner is a motion photo → 3 documents: depth 1 image/jpeg named
motion-photo.mp4, depth 2 its own video/mp4. Chained 1500 deep (~1.4 KB per
link, 2.19 MB input), a plain
AutoDetectParser.parse → StackOverflowError.
The depth cap that saves the /rmeta path is
AbstractRecursiveParserWrapperHandler.MAX_DEPTH = 100, which only exists on the
RecursiveParserWrapper path. ParseRecord.maxEmbeddedDepth defaults to -1, so
/tika, tika-app text output and the Tika facade have no
guard. Even at the capped 100 levels each level re-spools the whole
remaining suffix via tis.getPath(), so a 50 MB input writes gigabytes of temp
files.
Fix is one line and is what the PR already claims: require
"video".equals(type.getType()). It matches both specs (the item's semantic is
the video), and it closes the recursion for good, since a video parser never
re-enters this code. Add the missing test —
a trailer that is recognized but not a video; testDeclaredVideoIsNotOne
only covers unrecognized zeros.
2. edge-case — extension from the declaration, content type from detection
MotionPhoto.java:221-233. The two can disagree (motion-photo.mp4 on
text/plain, above). The PR's rationale — "the parse that follows knows the
format for certain, but by then the name is fixed" — is about the parse; the
detected type is already in hand at
line 221, before the name is set at 229. Also
RESOURCE_NAME_EXTENSION_INFERRED=true is documented as "inferred by Tika (e.g.
from content type detection) rather than provided by the original document",
and here it is the document's declaration. If the
declaration is deliberately preferred, worth saying why detection-at-221
was rejected.
3. edge-case (low) — IOException on the video path fails the whole image
file(tis) maps getPath()'s IOException to null, but Files.size (:217),
detect (:221) and region (:239) propagate out of the image parser. That
contradicts the stated contract ("a video out of reach is no reason to fail an
image that parsed"). Cheap fix:
extend the guard over the rest of the body.
Hygiene
- MediaType.parse(metadata.get(... "Item:Mime")) (:289) interns an
attacker-controlled string into the static, never-evicted
MediaType.SIMPLE_TYPES — the same class of thing TIKA-4826/4862 are removing.
The value is only used for an extension, and
EmbeddedDocumentUtil.getExtensionForMediaType(String) already takes a
String and normalizes itself; dropping the parse removes the interning and a
field.
- CHANGES.txt "the bytes there are not a video" — fixed by finding 1.
- parseEmbedded(..., outputHtml=false) with no <div class="embedded"> of
its own; EMFParser, the closest precedent, passes true.
- 1583 is hardcoded in four places across two tests; derive it from the
fixture.
```
> Emit the video of a motion photo as an embedded document
> --------------------------------------------------------
>
> Key: TIKA-4869
> URL: https://issues.apache.org/jira/browse/TIKA-4869
> Project: Tika
> Issue Type: Improvement
> Reporter: Dominik Schmidt
> Priority: Major
>
> Tika exposes the Motion Photo and MicroVideo XMP of Google/Android motion
> photos, but not the video itself, although both formats say where it is and
> every byte of it is already in the file.
> The MP4 is appended after the JPEG:
> - Motion Photo (Camera:MotionPhoto): Container:Directory lists the items in
> file order, the primary image first, the rest tightly packed after it, each
> with an Item:Length and an optional Item:Padding. The video starts at
> fileLength minus the lengths of the items from the video to the end; in
> practice it is the last one, also in Ultra HDR files, where the spec puts the
> gain map before it.
> - MicroVideo (the older Camera:MicroVideo / GCamera): Camera:MicroVideoOffset
> is the number of bytes from the end of the file, so the video starts at
> fileLength minus that value.
> Proposal: the image parsers emit those bytes as an embedded document,
> ATTACHMENT, named motion-photo.<ext>. What is at the computed offset is
> detected by content, without the declared Item:Mime as a hint (a hint would
> make a wrong length pass as video/mp4 anyway), and nothing is emitted when
> detection finds no type: a wrong or hostile length then yields no embedded
> document rather than a bogus one. Extraction follows the usual embedded
> document limits; a client that does not want the video filters by type as
> usual.
> The same computation covers HEIC and AVIF motion photos, whose XMP goes
> through the same extractor: there the video sits in an mpvd box, but that box
> is last and its 8 byte header is the primary item's Padding, so the video
> still ends at the end of the file. Putting the extraction in the shared image
> parser base rather than in JpegParser therefore covers them too, untested for
> lack of a sample file.
> Apple Live Photos are a different thing and need nothing here: their video is
> a separate .MOV, paired by a content identifier, so a HEIC alone carries no
> video.
> The existing test files (testJPEG_MotionPhoto.jpg, testJPEG_MicroVideo.jpg)
> declare a video in their XMP but do not contain one; new fixtures with a
> small real MP4 appended are needed.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
