[
https://issues.apache.org/jira/browse/TIKA-4985?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18129646#comment-18129646
]
ASF GitHub Bot commented on TIKA-4985:
--------------------------------------
Copilot commented on code in PR #3377:
URL: https://github.com/apache/tika/pull/3377#discussion_r4337187884
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pkg-module/src/main/java/org/apache/tika/parser/pkg/RarParser.java:
##########
@@ -91,11 +93,17 @@ public void parse(TikaInputStream tis, ContentHandler
handler, Metadata metadata
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.CREATED, header.getCTime());
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.MODIFIED, header.getMTime());
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.ACCESSED, header.getATime());
- try (TikaInputStream rarTis =
TikaInputStream.get(rar.getInputStream(header))) {
+ EntryStream entryStream = new
EntryStream(rar.getInputStream(header));
+ try (TikaInputStream rarTis =
TikaInputStream.get(entryStream)) {
if (extractor.shouldParseEmbedded(entrydata, context))
{
extractor.parseEmbedded(rarTis, handler,
entrydata, context, true);
}
}
+ if (entryStream.failure != null) {
+ EmbeddedDocumentUtil.recordEmbeddedStreamException(
+ new IOException("RAR entry " +
header.getFileName(),
+ entryStream.failure), metadata,
context);
+ }
Review Comment:
If `TikaInputStream.get(entryStream)` throws during resource initialization,
`entryStream` will not be closed, potentially leaking the underlying junrar
stream. Consider using a multi-resource try-with-resources that includes
`EntryStream` itself (declared in the try header) so it will be closed even if
wrapping fails.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pkg-module/src/main/java/org/apache/tika/parser/pkg/RarParser.java:
##########
@@ -112,4 +120,24 @@ public void parse(TikaInputStream tis, ContentHandler
handler, Metadata metadata
xhtml.endDocument();
}
+
+ /**
+ * junrar 8.1.2+ reports a failed entry (truncated archive, bad CRC,
missing password) as an
+ * IOException on the entry's stream. End the entry there instead, as
junrar 8.1.1 did, so
+ * the bytes it delivered are still parsed and the rest of the archive is
reached.
+ */
+ private static class EntryStream extends ProxyInputStream {
+ private IOException failure;
+
+ EntryStream(InputStream in) {
+ super(in);
+ }
+
+ @Override
+ protected void handleIOException(IOException e) {
+ if (failure == null) {
+ failure = e;
+ }
+ }
Review Comment:
After the first failure is captured, subsequent reads may continue calling
into the underlying junrar stream, repeatedly hitting exceptions (even though
`ProxyInputStream` will convert them to EOF). To avoid repeated exception
overhead and reduce noise, consider short-circuiting reads after the first
failure (e.g., replacing the delegate with a closed/empty stream or tracking a
terminal state so future reads return EOF without touching the underlying
stream).
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pkg-module/src/test/java/org/apache/tika/parser/pkg/RarParserTest.java:
##########
@@ -92,6 +99,31 @@ public void testEmbedded() throws Exception {
assertContains("test-documents/testXML.xml", content);
}
+ @Test
+ public void testCorruptEntryKeepsLaterEntries(@TempDir Path tmp) throws
Exception {
+ byte[] rar;
+ try (TikaInputStream tis =
getResourceAsStream("/test-documents/test-documents.rar")) {
+ rar = IOUtils.toByteArray(tis);
+ }
+ // testEXCEL.xls, the first entry, has its packed data at 80..2885
+ rar[1500] ^= 0x5A;
Review Comment:
This test depends on hard-coded byte offsets within `test-documents.rar`,
which makes it fragile if that fixture changes (even slightly) or if the RAR
layout differs across rebuilds of the test artifact. A more robust approach
would be to programmatically locate the first entry's packed data range (or its
CRC) and corrupt within that computed range, so the test remains stable if the
archive contents are regenerated.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pkg-module/src/main/java/org/apache/tika/parser/pkg/RarParser.java:
##########
@@ -91,11 +93,17 @@ public void parse(TikaInputStream tis, ContentHandler
handler, Metadata metadata
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.CREATED, header.getCTime());
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.MODIFIED, header.getMTime());
AbstractArchiveParser.setLocalTime(entrydata,
FileSystem.ACCESSED, header.getATime());
- try (TikaInputStream rarTis =
TikaInputStream.get(rar.getInputStream(header))) {
+ EntryStream entryStream = new
EntryStream(rar.getInputStream(header));
+ try (TikaInputStream rarTis =
TikaInputStream.get(entryStream)) {
if (extractor.shouldParseEmbedded(entrydata, context))
{
extractor.parseEmbedded(rarTis, handler,
entrydata, context, true);
}
}
+ if (entryStream.failure != null) {
+ EmbeddedDocumentUtil.recordEmbeddedStreamException(
+ new IOException("RAR entry " +
header.getFileName(),
+ entryStream.failure), metadata,
context);
Review Comment:
The recorded exception message only includes the entry name, which can be
hard to interpret when aggregated. Consider including a clearer action/context
(e.g., that extraction failed mid-stream) and/or the underlying failure message
so users can quickly distinguish corruption vs. password/CRC/etc. without
having to inspect nested causes.
> Prep for 4.2.0 release
> ----------------------
>
> Key: TIKA-4985
> URL: https://issues.apache.org/jira/browse/TIKA-4985
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Major
>
> From the first round: junrar changed exception handling. The updated zip bomb
> detection triggers on compressed log files, and a few other fixes. Vast
> majority of content is unchanged. the mhtml change didn't do much. The html
> detection change is a clear improvement.
>
> Will run round two later today and post results if they're good enough for an
> rc1.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
