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.
--
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]
