review feedback

- if metadata length == 0, fail with specific error. With tests
- add tests for rejection of version != 1
- use faster readUnsignedLittleEndian() in validation knowing metadata is
  always little-endian

Author: Steve Loughran

parquet-variant/src/main/java/org/apache/parquet/variant/Variant.java

@@ -72,9 +72,8 @@ public final class Variant {
   static final int BINARY_SEARCH_THRESHOLD = 32;
 
   /**
-   * Create a Variant from a tuple of (value, metadata) byte arrays..
-   * Includes validation of version and a validation of the toplevel
-   * structure.
+   * Create a Variant from a tuple of (value, metadata) byte arrays.
+   * Includes validation of the version and the top-level structure.
    * @param value value buffer
    * @param metadata metadata buffer
    * @throws UnsupportedOperationException if there is a version mismatch
@@ -87,8 +86,7 @@ public Variant(byte[] value, byte[] metadata) {
   /**
    * Create a Variant from a subset of the (value, metadata) buffers
    * supplied.
-   * Includes validation of version and a validation of the toplevel
-   * structure.
+   * Includes validation of the version and the toplevel structure.
    * @param value value buffer
    * @param valuePos offset where the value data begins
    * @param valueLength length of value data
@@ -108,8 +106,7 @@ public Variant(byte[] value, int valuePos, int valueLength, byte[] metadata, int
 
   /**
    * Create a Variant from a tuple of (value, metadata) buffers.
-   * Includes validation of version and a validation of the toplevel
-   * structure.
+   * Includes validation of the version and the toplevel structure.
    * @param value value buffer
    * @param metadata metadata buffer
    * @throws UnsupportedOperationException if there is a version mismatch
@@ -119,6 +116,8 @@ public Variant(ByteBuffer value, ByteBuffer metadata) {
     this.value = value.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN);
     this.metadata = metadata.asReadOnlyBuffer().order(ByteOrder.LITTLE_ENDIAN);
     this.depth = 0;
+    // A metadata buffer must contain at least the version byte
+    Preconditions.checkArgument(this.metadata.remaining() >= 1, "variant metadata is empty");
     // There is currently only one allowed version.
     if ((metadata.get(metadata.position()) & VariantUtil.VERSION_MASK) != VariantUtil.VERSION) {
       throw new UnsupportedOperationException(String.format(
@@ -129,18 +128,14 @@ public Variant(ByteBuffer value, ByteBuffer metadata) {
     // Pre-compute dictionary size for lazy metadata cache allocation.
     int pos = this.metadata.position();
     int metaOffsetSize = ((this.metadata.get(pos) >> 6) & 0x3) + 1;
-    if (this.metadata.remaining() > 1) {
-      Preconditions.checkArgument(
-          this.metadata.remaining() >= 1 + metaOffsetSize,
-          "variant metadata truncated: offsetSize=" + metaOffsetSize);
-      this.dictSize = VariantUtil.readUnsignedLittleEndian(this.metadata, pos + 1, metaOffsetSize);
-      long dictTableEnd = 1L + metaOffsetSize + ((long) this.dictSize + 1) * metaOffsetSize;
-      Preconditions.checkArgument(
-          dictTableEnd <= this.metadata.remaining(),
-          "variant metadata dictionary extends past buffer: dictSize=" + this.dictSize);
-    } else {
-      this.dictSize = 0;
-    }
+    Preconditions.checkArgument(
+        this.metadata.remaining() >= 1 + metaOffsetSize,
+        "variant metadata truncated: offsetSize=" + metaOffsetSize);
+    this.dictSize = VariantUtil.readUnsignedLittleEndian(this.metadata, pos + 1, metaOffsetSize);
+    long dictTableEnd = 1L + metaOffsetSize + ((long) this.dictSize + 1) * metaOffsetSize;
+    Preconditions.checkArgument(
+        dictTableEnd <= this.metadata.remaining(),
+        "variant metadata dictionary extends past buffer: dictSize=" + this.dictSize);
     this.metadataCache = null;
     VariantUtil.validateValueShallow(this.value, dictSize);
   }
@@ -407,6 +402,7 @@ public Variant getElementAtIndex(int index) {
 
   /**
    * Creates a child Variant that shares this instance's metadata cache.
+   * @throws IllegalArgumentException validation error during construction.
    */
   private Variant childVariant(ByteBuffer childValue) {
     return new Variant(childValue, metadata, metadataCache, dictSize, depth + 1);

parquet-variant/src/main/java/org/apache/parquet/variant/VariantUtil.java

@@ -950,7 +950,7 @@ private static void validateContainerShallow(
     int offsetSize = (typeInfo & 0x3) + 1;
     int sizeBytes = largeSize ? U32_SIZE : 1;
     Preconditions.checkArgument(1L + sizeBytes <= length, "variant container header truncated");
-    int numElements = readUnsigned(valueBuffer, pos + 1, sizeBytes);
+    int numElements = readUnsignedLittleEndian(valueBuffer, pos + 1, sizeBytes);
     long idStart = 1L + sizeBytes;
     long idBytes = isObject ? (long) numElements * idSize : 0L;
     long offsetStart = idStart + idBytes;
@@ -961,7 +961,7 @@ private static void validateContainerShallow(
     long dataLen = length - dataStart;
     if (isObject) {
       for (int i = 0; i < numElements; i++) {
-        int id = readUnsigned(valueBuffer, pos + (int) idStart + i * idSize, idSize);
+        int id = readUnsignedLittleEndian(valueBuffer, pos + (int) idStart + i * idSize, idSize);
         Preconditions.checkArgument(
             id < dictSize, "variant object key id %s out of range (dictSize=%s)", id, dictSize);
       }
@@ -970,7 +970,7 @@ private static void validateContainerShallow(
     // the trailing terminator offset is range-checked for the same reason.
     for (int i = 0; i <= numElements; i++) {
       // O(elements)
-      int off = readUnsigned(valueBuffer, pos + (int) offsetStart + i * offsetSize, offsetSize);
+      int off = readUnsignedLittleEndian(valueBuffer, pos + (int) offsetStart + i * offsetSize, offsetSize);
       Preconditions.checkArgument(
           off <= dataLen, "variant child offset out of range: %s (data length %s)", off, dataLen);
     }

parquet-variant/src/test/java/org/apache/parquet/variant/TestHardenedReader.java

@@ -310,6 +310,40 @@ public void testOversizedMetadataDictSize() throws IOException {
     expectRoundTripRaisesIllegalArgument(metadata, arrayOneNull(), "dictionary");
   }
 
+  /**
+   * Reject an empty metadata buffer.
+   */
+  @Test
+  public void testEmptyMetadataRejected() {
+    IllegalArgumentException thrown = assertThrows(
+        IllegalArgumentException.class,
+        () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[0])));
+    assertExceptionMessageContains(thrown, "empty");
+  }
+
+  /**
+   * Reject a single-byte metadata buffer.
+   */
+  @Test
+  public void testSingleByteMetadataRejected() {
+    IllegalArgumentException thrown = assertThrows(
+        IllegalArgumentException.class,
+        () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[] {0x01})));
+    assertExceptionMessageContains(thrown, "truncated");
+  }
+
+  /**
+   * Reject metadata whose version bits are not the single supported version.
+   */
+  @Test
+  public void testUnsupportedMetadataVersionRejected() {
+    // Header 0x02: version=2 in the low bits, offsetSize=1. A well-formed empty dictionary.
+    UnsupportedOperationException thrown = assertThrows(
+        UnsupportedOperationException.class,
+        () -> new Variant(ByteBuffer.wrap(arrayOneNull()), ByteBuffer.wrap(new byte[] {0x02, 0x00, 0x00})));
+    assertExceptionMessageContains(thrown, "version");
+  }
+
   // ------------------------------------------------------------------
   // Helpers
   // ------------------------------------------------------------------