tballison commented on code in PR #3130: URL: https://github.com/apache/tika/pull/3130#discussion_r3936781953
########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/detect/image/RawTiffDetectorFuzzTest.java: ########## @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tika.detect.image; + +import static org.junit.jupiter.api.Assertions.fail; + +import java.util.Locale; +import java.util.Random; + +import org.junit.jupiter.api.Test; + +/** + * Randomized boundary test for {@link RawTiffDetector}'s directory walk. + * <p> + * The detector runs ahead of every parser, and {@code Detector.detect} declares + * only {@link java.io.IOException}: anything else it throws aborts detection for + * the document and the remaining detectors never run. So the invariant is simply + * that no throwable escapes, whatever the directories say. + * <p> + * Inputs are well-formed TIFF and BigTIFF headers whose offsets, counts and + * entry values are drawn from the arithmetic boundaries -- 0, the 32 and 64 bit + * maxima, the prefix limit, and their neighbours -- since that is where the + * offset handling goes wrong rather than in random bytes. The seed is random per + * run and reported on failure. + */ +public class RawTiffDetectorFuzzTest { + + private static final int TRIALS = 20000; Review Comment: I measured before changing it: 20,000 trials of the generator plus `detect` on 24-424 byte arrays is ~40ms (200,000 is ~210ms) -- everything is in-memory over a few hundred bytes, and the detector bails at the first bad offset. That is noise next to surefire fork startup, so there is nothing for CI to dial down. The comparison to `TikaEvalTokenizerFuzzTest` is not per-trial equivalent: that one runs an analyzer over generated text, so 2,000 trials there costs far more than 20,000 here. The trial counts were each picked from the measured cost, not copied. Leaving the constant as is -- a system property would add a knob with no cost to control, and a fuzz count that CI can lower silently is worse than one that is fixed and cheap. ########## tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/test/java/org/apache/tika/detect/image/RawTiffDetectorTest.java: ########## @@ -224,6 +225,49 @@ public void testTruncatedPrefixIsHarmless() { } } + /** + * A BigTIFF directory offset near {@code Long.MAX_VALUE}: adding the entry + * count to it wraps negative, and a negative end must not read as "already + * in the prefix". The three pointer sources (this header field, a SubIFDs + * array, the follower below) all reach the same bounds check. + */ + @ParameterizedTest + @ValueSource(longs = {Long.MAX_VALUE, Long.MAX_VALUE - 7, Long.MAX_VALUE - 8, + 0x100000000L, 1024L * 1024L + 1}) + public void testDirectoryOffsetBeyondTheFileIsRejected(long firstIfd) throws Exception { + byte[] tiff = bigTiffHeader(firstIfd); + assertEquals(MediaType.OCTET_STREAM, RawTiffDetector.detect(tiff, tiff.length)); + try (TikaInputStream tis = TikaInputStream.get(tiff)) { + assertEquals(MediaType.OCTET_STREAM, + new RawTiffDetector().detect(tis, new Metadata(), new ParseContext())); + } + } + + /** + * The same offset as the follower of an otherwise good directory: the + * vendor the directory names still decides the type. + */ Review Comment: Fair -- "the vendor the directory names" is a garden path. Reworded to spell out both halves of what the test asserts: ``` /** * The same offset as the follower of an otherwise good directory: the * follower is skipped and the vendor named in the directory already read * still decides the type. */ ``` -- 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]
