steveloughran commented on code in PR #3562: URL: https://github.com/apache/parquet-java/pull/3562#discussion_r3560556669
########## parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java: ########## @@ -0,0 +1,746 @@ +/* + * 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.parquet.variant; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.io.ByteArrayOutputStream; +import java.io.EOFException; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.parquet.example.data.Group; +import org.apache.parquet.example.data.simple.SimpleGroup; +import org.apache.parquet.hadoop.ParquetReader; +import org.apache.parquet.hadoop.ParquetWriter; +import org.apache.parquet.hadoop.api.ReadSupport; +import org.apache.parquet.hadoop.example.ExampleParquetWriter; +import org.apache.parquet.hadoop.example.GroupReadSupport; +import org.apache.parquet.hadoop.metadata.CompressionCodecName; +import org.apache.parquet.io.InputFile; +import org.apache.parquet.io.OutputFile; +import org.apache.parquet.io.PositionOutputStream; +import org.apache.parquet.io.SeekableInputStream; +import org.apache.parquet.io.api.Binary; +import org.apache.parquet.schema.MessageType; +import org.apache.parquet.schema.MessageTypeParser; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestName; + +/** + * Exercises the bounds checks added to {@link Variant} construction by round-tripping a row of + * {@code (metadata, value)} binaries through an in-memory uncompressed Parquet file, mutating + * specific bytes after writing, and asserting that the resulting buffer is rejected with an + * {@link IllegalArgumentException}. + */ +public class TestHardenedReader { + + private static final String SCHEMA_STRING = + "message variant_record { required binary metadata; required binary value; }"; + + private static final MessageType SCHEMA = MessageTypeParser.parseMessageType(SCHEMA_STRING); + + /** + * Minimal well-formed empty metadata: version=1, offsetSize=1, dictSize=0, single end-offset=0. + */ + private static final byte[] EMPTY_METADATA = {0x01, 0x00, 0x00}; + + /** + * Set to {@code true} to persist the generated Parquet files under + * {@code target/test-hardened-reader/$testName.parquet}. + * This allows file to be added to the bad_data section of parquet-testing repository. + */ + private static final boolean SAVE_GENERATED_FILES = false; + + /** Directory under the module's build dir where generated files are written, if enabled. */ + private static final Path OUTPUT_DIR = Paths.get("target", "test-hardened-reader"); + + @Rule + public TestName testName = new TestName(); + + /** + * Happy path. + */ + @Test + public void testWellFormedRoundTrip() throws IOException { + byte[] wellFormed = arrayOneNull(); + byte[][] roundTripped = roundTrip(EMPTY_METADATA, wellFormed); + Variant v = new Variant(ByteBuffer.wrap(roundTripped[1]), ByteBuffer.wrap(roundTripped[0])); + assertEquals(Variant.Type.ARRAY, v.getType()); + assertEquals(1, v.numArrayElements()); + assertEquals(Variant.Type.NULL, v.getElementAtIndex(0).getType()); + } + + /** + * Out-of-range child offset. + */ + @Test + public void testOutOfRangeChildOffset() throws IOException { + byte[] value = arrayOneNull(); + // Layout of arrayOneNull(): [header, numElements=1, offset[0]=0, offset[1]=1, NULL_header] + // Set offset[0] to 0xFF — far outside the 1-byte data region. + value[2] = (byte) 0xFF; + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "child offset"); + } + + /** + * Reject numElements larger than the surrounding buffer. + */ + @Test + public void testOversizedNumElements() throws IOException { + // Use a largeSize array so numElements is a 4-byte field we can grow. + // Layout: [arrayHeader(large=1,offsetSize=1)=0b10011, numElements(4 bytes LE)=0, + // end-offset(1 byte)=0] — total 6 bytes, zero data. + byte[] value = new byte[] {0b10011, 0x00, 0x00, 0x00, 0x00, 0x00}; + // Baseline must parse cleanly. + new Variant(ByteBuffer.wrap(value.clone()), ByteBuffer.wrap(EMPTY_METADATA.clone())); + // Set numElements to a value whose offset table cannot fit in the buffer. + int huge = 0x10000000; + value[1] = (byte) (huge & 0xFF); + value[2] = (byte) ((huge >> 8) & 0xFF); + value[3] = (byte) ((huge >> 16) & 0xFF); + value[4] = (byte) ((huge >> 24) & 0xFF); + expectRoundTripRaisesIllegalArgument(EMPTY_METADATA, value, "offset table"); + } + + /** + * Reject Offset arithmetic that would wrap java's signed 32-bit integer. + */ + @Test + public void testOffsetArithmeticBoundsCheck() throws IOException { + // Object header that claims numElements and offsetSize values whose product overflows int + // unless validated with widened arithmetic. + // Layout: [objectHeader(large=1, idSize=1, offsetSize=4) = 0x4E, + // numElements (4 bytes, little-endian) = 0x33333333, + // three filler bytes] + byte[] value = new byte[] {0x4E, 0x33, 0x33, 0x33, 0x33, (byte) 0xFF, (byte) 0xFF, 0x3F}; Review Comment: ok, let's try to expand this, as it's the key one you are flagging up. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
