From 2fdec3d4a1260cb1630c73f770545e7c75a94510 Mon Sep 17 00:00:00 2001 From: Vova Kolmakov Date: Thu, 21 May 2026 11:58:36 +0700 Subject: [PATCH 1/3] Core, Data: Validate deletion-vector offset and length Hostile or corrupted manifest metadata can claim a negative or near-2 GiB `content_offset`/`content_size_in_bytes` on a deletion-vector `DeleteFile`. The existing precondition in `BaseDeleteLoader.validateDV` only checked non-null and `size <= Integer.MAX_VALUE`, so a negative size slipped through to `new byte[length]` and threw `NegativeArraySizeException`; a negative offset reached `IOUtil.readFully` and produced an invalid seek. The same unguarded allocation pattern lives in `DVUtil.readDV` on the rewrite/DV-merge path used by `mergeAndWriteDVsIfRequired`. Add `ContentFileUtil.validateDV` as the canonical read-path validator (null offset, null size, offset >= 0, size >= 0, size <= Integer.MAX_VALUE) and delegate from both `BaseDeleteLoader.validateDV` and `DVUtil.readDV`. `BaseDeleteLoader` keeps its loader-specific `referencedDataFile` check. For defense-in-depth, also reject negative `contentOffset`/`contentSizeInBytes` in `FileMetadata.Builder.build()` for `PUFFIN`, mirroring the existing `fileSizeInBytes >= 0` / `recordCount >= 0` checks. Producers using the builder can no longer accidentally emit bad metadata; the read-side check still defends against corrupted on-disk manifests where `BaseFile.internalSet` hydrates fields directly via Avro reflection. Tests cover the new validator end-to-end, the builder rejection, and an integration check that `BaseDeleteLoader.loadPositionDeletes` rejects a tampered DV before any I/O attempt. Closes #16475 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../main/java/org/apache/iceberg/DVUtil.java | 1 + .../java/org/apache/iceberg/FileMetadata.java | 6 ++ .../apache/iceberg/util/ContentFileUtil.java | 21 ++++ .../org/apache/iceberg/TestFileMetadata.java | 100 ++++++++++++++++++ .../iceberg/util/TestContentFileUtil.java | 97 +++++++++++++++++ .../apache/iceberg/data/BaseDeleteLoader.java | 13 +-- .../apache/iceberg/TestBaseDeleteLoader.java | 86 +++++++++++++++ 7 files changed, 312 insertions(+), 12 deletions(-) create mode 100644 core/src/test/java/org/apache/iceberg/TestFileMetadata.java create mode 100644 core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java create mode 100644 data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java diff --git a/core/src/main/java/org/apache/iceberg/DVUtil.java b/core/src/main/java/org/apache/iceberg/DVUtil.java index c323e96775fc..a5bfc4dcf541 100644 --- a/core/src/main/java/org/apache/iceberg/DVUtil.java +++ b/core/src/main/java/org/apache/iceberg/DVUtil.java @@ -52,6 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) { ContentFileUtil.isDV(deleteFile), "Cannot read, not a deletion vector: %s", deleteFile.location()); + ContentFileUtil.validateDV(deleteFile); InputFile inputFile = fileIO.newInputFile(deleteFile); long offset = deleteFile.contentOffset(); int length = deleteFile.contentSizeInBytes().intValue(); diff --git a/core/src/main/java/org/apache/iceberg/FileMetadata.java b/core/src/main/java/org/apache/iceberg/FileMetadata.java index a5266101c252..f8fa8b7ee704 100644 --- a/core/src/main/java/org/apache/iceberg/FileMetadata.java +++ b/core/src/main/java/org/apache/iceberg/FileMetadata.java @@ -263,6 +263,12 @@ public DeleteFile build() { if (format == FileFormat.PUFFIN) { Preconditions.checkArgument(contentOffset != null, "Content offset is required for DV"); Preconditions.checkArgument(contentSizeInBytes != null, "Content size is required for DV"); + Preconditions.checkArgument( + contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset); + Preconditions.checkArgument( + contentSizeInBytes >= 0, + "Content size must be non-negative for DV: %s", + contentSizeInBytes); Preconditions.checkArgument( referencedDataFile != null, "Referenced data file is required for DV"); } else { diff --git a/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java b/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java index ac7de6644c51..419a5009d28d 100644 --- a/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java @@ -156,6 +156,27 @@ public static String dvDesc(DeleteFile deleteFile) { deleteFile.referencedDataFile()); } + /** + * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed + * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise + * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation + * when the DV blob is read. + */ + public static void validateDV(DeleteFile dv) { + Preconditions.checkArgument( + dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv)); + Preconditions.checkArgument( + dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() <= Integer.MAX_VALUE, + "Can't read DV larger than 2GB: %s", + dv.contentSizeInBytes()); + } + private static Metrics metricsWithoutPathBounds(DeleteFile file) { Map lowerBounds = file.lowerBounds() == null ? null : Maps.newHashMap(file.lowerBounds()); diff --git a/core/src/test/java/org/apache/iceberg/TestFileMetadata.java b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java new file mode 100644 index 000000000000..740b8a4cad8f --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java @@ -0,0 +1,100 @@ +/* + * 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.iceberg; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +public class TestFileMetadata { + + @Test + public void dvBuilderRejectsNegativeContentOffset() { + assertThatThrownBy( + () -> + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(-1L) + .withContentSizeInBytes(10L) + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Content offset must be non-negative for DV"); + } + + @Test + public void dvBuilderRejectsNegativeContentSize() { + assertThatThrownBy( + () -> + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(0L) + .withContentSizeInBytes(-1L) + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Content size must be non-negative for DV"); + } + + @Test + public void dvBuilderAcceptsValidOffsetAndSize() { + DeleteFile dv = + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(4L) + .withContentSizeInBytes(4096L) + .build(); + + assertThat(dv.contentOffset()).isEqualTo(4L); + assertThat(dv.contentSizeInBytes()).isEqualTo(4096L); + assertThat(dv.format()).isEqualTo(FileFormat.PUFFIN); + } + + @Test + public void dvBuilderAcceptsZeroOffsetAndSize() { + assertThatCode( + () -> + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(0L) + .withContentSizeInBytes(0L) + .build()) + .doesNotThrowAnyException(); + } +} diff --git a/core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java b/core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java new file mode 100644 index 000000000000..8a08d8415ce7 --- /dev/null +++ b/core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java @@ -0,0 +1,97 @@ +/* + * 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.iceberg.util; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import org.apache.iceberg.DeleteFile; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + @Test + public void validateDVRejectsNullOffset() { + DeleteFile dv = dv(null, 10L); + assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("offset cannot be null"); + } + + @Test + public void validateDVRejectsNullLength() { + DeleteFile dv = dv(0L, null); + assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("length cannot be null"); + } + + @Test + public void validateDVRejectsNegativeOffset() { + DeleteFile dv = dv(-1L, 10L); + assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("offset must be non-negative"); + } + + @Test + public void validateDVRejectsNegativeLength() { + DeleteFile dv = dv(0L, -1L); + assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("length must be non-negative"); + } + + @Test + public void validateDVRejectsLengthAboveIntegerMax() { + DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE + 1); + assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Can't read DV larger than 2GB"); + } + + @Test + public void validateDVAcceptsZero() { + DeleteFile dv = dv(0L, 0L); + assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + } + + @Test + public void validateDVAcceptsTypicalValues() { + DeleteFile dv = dv(4L, 4096L); + assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + } + + @Test + public void validateDVAcceptsMaximumLength() { + DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE); + assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + } + + private static DeleteFile dv(Long offset, Long length) { + DeleteFile dv = mock(DeleteFile.class); + when(dv.location()).thenReturn("/tmp/test.puffin"); + when(dv.referencedDataFile()).thenReturn("/tmp/data.parquet"); + when(dv.contentOffset()).thenReturn(offset); + when(dv.contentSizeInBytes()).thenReturn(length); + return dv; + } +} diff --git a/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java b/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java index 02b06b70e483..cfc60b3d431f 100644 --- a/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java +++ b/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java @@ -264,18 +264,7 @@ private int estimateRecordSize(Schema schema) { } private void validateDV(DeleteFile dv, CharSequence filePath) { - Preconditions.checkArgument( - dv.contentOffset() != null, - "Invalid DV, offset cannot be null: %s", - ContentFileUtil.dvDesc(dv)); - Preconditions.checkArgument( - dv.contentSizeInBytes() != null, - "Invalid DV, length is null: %s", - ContentFileUtil.dvDesc(dv)); - Preconditions.checkArgument( - dv.contentSizeInBytes() <= Integer.MAX_VALUE, - "Can't read DV larger than 2GB: %s", - dv.contentSizeInBytes()); + ContentFileUtil.validateDV(dv); Preconditions.checkArgument( filePath.toString().equals(dv.referencedDataFile()), "DV is expected to reference %s, not %s", diff --git a/data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java b/data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java new file mode 100644 index 000000000000..556e6c4ebc1a --- /dev/null +++ b/data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java @@ -0,0 +1,86 @@ +/* + * 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.iceberg; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.apache.iceberg.data.BaseDeleteLoader; +import org.apache.iceberg.data.DeleteLoader; +import org.apache.iceberg.io.InputFile; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.Test; + +public class TestBaseDeleteLoader { + + private static final String DATA_FILE = "/tmp/data.parquet"; + + private static final DeleteFile VALID_DV = + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile(DATA_FILE) + .withContentOffset(0L) + .withContentSizeInBytes(10L) + .build(); + + @Test + public void loadPositionDeletesRejectsNegativeContentOffset() { + DeleteFile dv = tamperedDV(-1L, 10L); + DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile()); + + assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("offset must be non-negative"); + } + + @Test + public void loadPositionDeletesRejectsNegativeContentSize() { + DeleteFile dv = tamperedDV(0L, -1L); + DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile()); + + assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("length must be non-negative"); + } + + // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest, + // since the FileMetadata builder now rejects negative values at construction time. + private static DeleteFile tamperedDV(Long offset, Long size) { + return new Delegates.DelegatingDeleteFile(VALID_DV) { + @Override + public Long contentOffset() { + return offset; + } + + @Override + public Long contentSizeInBytes() { + return size; + } + }; + } + + // The validator must fire before any I/O attempt — if it does not, the loader will call this + // and the test will surface that as a clear failure instead of an obscure stack trace. + private static InputFile failingInputFile() { + throw new AssertionError("DV validation should reject the tampered metadata before any I/O"); + } +} From 30256ab67e9de20c2d7a391959283e25aa9cec6b Mon Sep 17 00:00:00 2001 From: Vova Kolmakov Date: Thu, 28 May 2026 09:36:08 +0700 Subject: [PATCH 2/3] Core, Data: Move DV validation to DVUtil and tighten size bound Co-Authored-By: Claude Opus 4.7 (1M context) --- .../main/java/org/apache/iceberg/DVUtil.java | 33 ++++++++++- .../java/org/apache/iceberg/FileMetadata.java | 4 ++ .../apache/iceberg/util/ContentFileUtil.java | 21 ------- ...stContentFileUtil.java => TestDVUtil.java} | 31 +++++++---- .../org/apache/iceberg/TestFileMetadata.java | 18 ++++++ .../apache/iceberg/data/BaseDeleteLoader.java | 3 +- .../{ => data}/TestBaseDeleteLoader.java | 55 +++++++------------ 7 files changed, 95 insertions(+), 70 deletions(-) rename core/src/test/java/org/apache/iceberg/{util/TestContentFileUtil.java => TestDVUtil.java} (76%) rename data/src/test/java/org/apache/iceberg/{ => data}/TestBaseDeleteLoader.java (57%) diff --git a/core/src/main/java/org/apache/iceberg/DVUtil.java b/core/src/main/java/org/apache/iceberg/DVUtil.java index a5bfc4dcf541..fa8ca2690681 100644 --- a/core/src/main/java/org/apache/iceberg/DVUtil.java +++ b/core/src/main/java/org/apache/iceberg/DVUtil.java @@ -44,7 +44,7 @@ import org.apache.iceberg.util.Pair; import org.apache.iceberg.util.Tasks; -class DVUtil { +public class DVUtil { private DVUtil() {} static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) { @@ -52,7 +52,7 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) { ContentFileUtil.isDV(deleteFile), "Cannot read, not a deletion vector: %s", deleteFile.location()); - ContentFileUtil.validateDV(deleteFile); + validateDV(deleteFile); InputFile inputFile = fileIO.newInputFile(deleteFile); long offset = deleteFile.contentOffset(); int length = deleteFile.contentSizeInBytes().intValue(); @@ -65,6 +65,35 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) { } } + /** + * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed + * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise + * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation + * when the DV blob is read. + */ + public static void validateDV(DeleteFile dv) { + Preconditions.checkArgument( + dv.contentOffset() != null, + "Invalid DV, offset cannot be null: %s", + ContentFileUtil.dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() != null, + "Invalid DV, length cannot be null: %s", + ContentFileUtil.dvDesc(dv)); + Preconditions.checkArgument( + dv.contentOffset() >= 0, + "Invalid DV, offset must be non-negative: %s", + ContentFileUtil.dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() >= 0, + "Invalid DV, length must be non-negative: %s", + ContentFileUtil.dvDesc(dv)); + Preconditions.checkArgument( + dv.contentSizeInBytes() < Integer.MAX_VALUE, + "Can't read DV larger than 2GB: %s", + dv.contentSizeInBytes()); + } + /** * Merges duplicate DVs for the same data file and writes the merged DV Puffin files. If there is * exactly 1 DV for a given data file then it is return as is diff --git a/core/src/main/java/org/apache/iceberg/FileMetadata.java b/core/src/main/java/org/apache/iceberg/FileMetadata.java index f8fa8b7ee704..d7c18ef751d6 100644 --- a/core/src/main/java/org/apache/iceberg/FileMetadata.java +++ b/core/src/main/java/org/apache/iceberg/FileMetadata.java @@ -269,6 +269,10 @@ public DeleteFile build() { contentSizeInBytes >= 0, "Content size must be non-negative for DV: %s", contentSizeInBytes); + Preconditions.checkArgument( + contentSizeInBytes < Integer.MAX_VALUE, + "Content size must be less than 2GB for DV: %s", + contentSizeInBytes); Preconditions.checkArgument( referencedDataFile != null, "Referenced data file is required for DV"); } else { diff --git a/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java b/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java index 419a5009d28d..ac7de6644c51 100644 --- a/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java +++ b/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java @@ -156,27 +156,6 @@ public static String dvDesc(DeleteFile deleteFile) { deleteFile.referencedDataFile()); } - /** - * Validates that the deletion-vector offset and length on a {@link DeleteFile} are well-formed - * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise - * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation - * when the DV blob is read. - */ - public static void validateDV(DeleteFile dv) { - Preconditions.checkArgument( - dv.contentOffset() != null, "Invalid DV, offset cannot be null: %s", dvDesc(dv)); - Preconditions.checkArgument( - dv.contentSizeInBytes() != null, "Invalid DV, length cannot be null: %s", dvDesc(dv)); - Preconditions.checkArgument( - dv.contentOffset() >= 0, "Invalid DV, offset must be non-negative: %s", dvDesc(dv)); - Preconditions.checkArgument( - dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv)); - Preconditions.checkArgument( - dv.contentSizeInBytes() <= Integer.MAX_VALUE, - "Can't read DV larger than 2GB: %s", - dv.contentSizeInBytes()); - } - private static Metrics metricsWithoutPathBounds(DeleteFile file) { Map lowerBounds = file.lowerBounds() == null ? null : Maps.newHashMap(file.lowerBounds()); diff --git a/core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java b/core/src/test/java/org/apache/iceberg/TestDVUtil.java similarity index 76% rename from core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java rename to core/src/test/java/org/apache/iceberg/TestDVUtil.java index 8a08d8415ce7..fe0e566b6767 100644 --- a/core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java +++ b/core/src/test/java/org/apache/iceberg/TestDVUtil.java @@ -16,22 +16,21 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.iceberg.util; +package org.apache.iceberg; import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import org.apache.iceberg.DeleteFile; import org.junit.jupiter.api.Test; -public class TestContentFileUtil { +public class TestDVUtil { @Test public void validateDVRejectsNullOffset() { DeleteFile dv = dv(null, 10L); - assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + assertThatThrownBy(() -> DVUtil.validateDV(dv)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("offset cannot be null"); } @@ -39,7 +38,7 @@ public void validateDVRejectsNullOffset() { @Test public void validateDVRejectsNullLength() { DeleteFile dv = dv(0L, null); - assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + assertThatThrownBy(() -> DVUtil.validateDV(dv)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("length cannot be null"); } @@ -47,7 +46,7 @@ public void validateDVRejectsNullLength() { @Test public void validateDVRejectsNegativeOffset() { DeleteFile dv = dv(-1L, 10L); - assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + assertThatThrownBy(() -> DVUtil.validateDV(dv)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("offset must be non-negative"); } @@ -55,15 +54,23 @@ public void validateDVRejectsNegativeOffset() { @Test public void validateDVRejectsNegativeLength() { DeleteFile dv = dv(0L, -1L); - assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + assertThatThrownBy(() -> DVUtil.validateDV(dv)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("length must be non-negative"); } + @Test + public void validateDVRejectsLengthEqualToIntegerMax() { + DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE); + assertThatThrownBy(() -> DVUtil.validateDV(dv)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Can't read DV larger than 2GB"); + } + @Test public void validateDVRejectsLengthAboveIntegerMax() { DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE + 1); - assertThatThrownBy(() -> ContentFileUtil.validateDV(dv)) + assertThatThrownBy(() -> DVUtil.validateDV(dv)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Can't read DV larger than 2GB"); } @@ -71,19 +78,19 @@ public void validateDVRejectsLengthAboveIntegerMax() { @Test public void validateDVAcceptsZero() { DeleteFile dv = dv(0L, 0L); - assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + assertThatCode(() -> DVUtil.validateDV(dv)).doesNotThrowAnyException(); } @Test public void validateDVAcceptsTypicalValues() { DeleteFile dv = dv(4L, 4096L); - assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + assertThatCode(() -> DVUtil.validateDV(dv)).doesNotThrowAnyException(); } @Test public void validateDVAcceptsMaximumLength() { - DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE); - assertThatCode(() -> ContentFileUtil.validateDV(dv)).doesNotThrowAnyException(); + DeleteFile dv = dv(0L, (long) Integer.MAX_VALUE - 1); + assertThatCode(() -> DVUtil.validateDV(dv)).doesNotThrowAnyException(); } private static DeleteFile dv(Long offset, Long length) { diff --git a/core/src/test/java/org/apache/iceberg/TestFileMetadata.java b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java index 740b8a4cad8f..fe4e0226f80a 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileMetadata.java +++ b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java @@ -62,6 +62,24 @@ public void dvBuilderRejectsNegativeContentSize() { .hasMessageContaining("Content size must be non-negative for DV"); } + @Test + public void dvBuilderRejectsContentSizeAtIntegerMax() { + assertThatThrownBy( + () -> + FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(0L) + .withContentSizeInBytes(Integer.MAX_VALUE) + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("less than 2GB"); + } + @Test public void dvBuilderAcceptsValidOffsetAndSize() { DeleteFile dv = diff --git a/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java b/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java index cfc60b3d431f..903a067efbab 100644 --- a/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java +++ b/data/src/main/java/org/apache/iceberg/data/BaseDeleteLoader.java @@ -25,6 +25,7 @@ import java.util.concurrent.ExecutorService; import java.util.function.Function; import java.util.function.Supplier; +import org.apache.iceberg.DVUtil; import org.apache.iceberg.DeleteFile; import org.apache.iceberg.FileFormat; import org.apache.iceberg.MetadataColumns; @@ -264,7 +265,7 @@ private int estimateRecordSize(Schema schema) { } private void validateDV(DeleteFile dv, CharSequence filePath) { - ContentFileUtil.validateDV(dv); + DVUtil.validateDV(dv); Preconditions.checkArgument( filePath.toString().equals(dv.referencedDataFile()), "DV is expected to reference %s, not %s", diff --git a/data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java b/data/src/test/java/org/apache/iceberg/data/TestBaseDeleteLoader.java similarity index 57% rename from data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java rename to data/src/test/java/org/apache/iceberg/data/TestBaseDeleteLoader.java index 556e6c4ebc1a..c1414c868c8a 100644 --- a/data/src/test/java/org/apache/iceberg/TestBaseDeleteLoader.java +++ b/data/src/test/java/org/apache/iceberg/data/TestBaseDeleteLoader.java @@ -16,12 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.iceberg; +package org.apache.iceberg.data; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; -import org.apache.iceberg.data.BaseDeleteLoader; -import org.apache.iceberg.data.DeleteLoader; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileFormat; import org.apache.iceberg.io.InputFile; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; import org.junit.jupiter.api.Test; @@ -30,21 +32,9 @@ public class TestBaseDeleteLoader { private static final String DATA_FILE = "/tmp/data.parquet"; - private static final DeleteFile VALID_DV = - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile(DATA_FILE) - .withContentOffset(0L) - .withContentSizeInBytes(10L) - .build(); - @Test public void loadPositionDeletesRejectsNegativeContentOffset() { - DeleteFile dv = tamperedDV(-1L, 10L); + DeleteFile dv = dv(-1L, 10L); DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile()); assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE)) @@ -54,7 +44,7 @@ public void loadPositionDeletesRejectsNegativeContentOffset() { @Test public void loadPositionDeletesRejectsNegativeContentSize() { - DeleteFile dv = tamperedDV(0L, -1L); + DeleteFile dv = dv(0L, -1L); DeleteLoader loader = new BaseDeleteLoader(file -> failingInputFile()); assertThatThrownBy(() -> loader.loadPositionDeletes(ImmutableList.of(dv), DATA_FILE)) @@ -62,25 +52,22 @@ public void loadPositionDeletesRejectsNegativeContentSize() { .hasMessageContaining("length must be non-negative"); } - // Wraps a well-formed DV but reports tampered (offset, size). Simulates a corrupted manifest, - // since the FileMetadata builder now rejects negative values at construction time. - private static DeleteFile tamperedDV(Long offset, Long size) { - return new Delegates.DelegatingDeleteFile(VALID_DV) { - @Override - public Long contentOffset() { - return offset; - } - - @Override - public Long contentSizeInBytes() { - return size; - } - }; + // Returns a DV mock reporting the given (offset, size). A mock bypasses the FileMetadata builder, + // which now rejects negative values at construction time, so it stands in for a corrupted + // manifest. + private static DeleteFile dv(Long offset, Long size) { + DeleteFile dv = mock(DeleteFile.class); + when(dv.format()).thenReturn(FileFormat.PUFFIN); + when(dv.location()).thenReturn("/tmp/dv.puffin"); + when(dv.referencedDataFile()).thenReturn(DATA_FILE); + when(dv.contentOffset()).thenReturn(offset); + when(dv.contentSizeInBytes()).thenReturn(size); + return dv; } - // The validator must fire before any I/O attempt — if it does not, the loader will call this - // and the test will surface that as a clear failure instead of an obscure stack trace. + // Validation must fire before any I/O. If it does not, the loader calls this and the test fails + // with a clear message instead of an obscure stack trace. private static InputFile failingInputFile() { - throw new AssertionError("DV validation should reject the tampered metadata before any I/O"); + throw new AssertionError("DV validation should reject the invalid metadata before any I/O"); } } From 00d778384dcbf3c48e699ebf45c171c0bfa71779 Mon Sep 17 00:00:00 2001 From: Vova Kolmakov Date: Tue, 2 Jun 2026 09:08:20 +0700 Subject: [PATCH 3/3] Core: Document DV validation exception and dedupe DV builder tests Co-Authored-By: Claude Opus 4.8 (1M context) --- .../main/java/org/apache/iceberg/DVUtil.java | 3 + .../org/apache/iceberg/TestFileMetadata.java | 77 ++++--------------- 2 files changed, 20 insertions(+), 60 deletions(-) diff --git a/core/src/main/java/org/apache/iceberg/DVUtil.java b/core/src/main/java/org/apache/iceberg/DVUtil.java index fa8ca2690681..fa5df9fb9945 100644 --- a/core/src/main/java/org/apache/iceberg/DVUtil.java +++ b/core/src/main/java/org/apache/iceberg/DVUtil.java @@ -70,6 +70,9 @@ static PositionDeleteIndex readDV(DeleteFile deleteFile, FileIO fileIO) { * before they are consumed by a reader. Hostile or corrupted manifest metadata may otherwise * trigger a {@link NegativeArraySizeException}, an invalid seek, or a multi-gigabyte allocation * when the DV blob is read. + * + * @throws IllegalArgumentException if the offset or length is null or negative, or the length is + * not less than 2GB */ public static void validateDV(DeleteFile dv) { Preconditions.checkArgument( diff --git a/core/src/test/java/org/apache/iceberg/TestFileMetadata.java b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java index fe4e0226f80a..e64340970107 100644 --- a/core/src/test/java/org/apache/iceberg/TestFileMetadata.java +++ b/core/src/test/java/org/apache/iceberg/TestFileMetadata.java @@ -28,71 +28,28 @@ public class TestFileMetadata { @Test public void dvBuilderRejectsNegativeContentOffset() { - assertThatThrownBy( - () -> - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile("/tmp/data.parquet") - .withContentOffset(-1L) - .withContentSizeInBytes(10L) - .build()) + assertThatThrownBy(() -> validDvBuilder().withContentOffset(-1L).build()) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Content offset must be non-negative for DV"); } @Test public void dvBuilderRejectsNegativeContentSize() { - assertThatThrownBy( - () -> - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile("/tmp/data.parquet") - .withContentOffset(0L) - .withContentSizeInBytes(-1L) - .build()) + assertThatThrownBy(() -> validDvBuilder().withContentSizeInBytes(-1L).build()) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("Content size must be non-negative for DV"); } @Test public void dvBuilderRejectsContentSizeAtIntegerMax() { - assertThatThrownBy( - () -> - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile("/tmp/data.parquet") - .withContentOffset(0L) - .withContentSizeInBytes(Integer.MAX_VALUE) - .build()) + assertThatThrownBy(() -> validDvBuilder().withContentSizeInBytes(Integer.MAX_VALUE).build()) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("less than 2GB"); } @Test public void dvBuilderAcceptsValidOffsetAndSize() { - DeleteFile dv = - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile("/tmp/data.parquet") - .withContentOffset(4L) - .withContentSizeInBytes(4096L) - .build(); + DeleteFile dv = validDvBuilder().withContentOffset(4L).withContentSizeInBytes(4096L).build(); assertThat(dv.contentOffset()).isEqualTo(4L); assertThat(dv.contentSizeInBytes()).isEqualTo(4096L); @@ -101,18 +58,18 @@ public void dvBuilderAcceptsValidOffsetAndSize() { @Test public void dvBuilderAcceptsZeroOffsetAndSize() { - assertThatCode( - () -> - FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) - .ofPositionDeletes() - .withFormat(FileFormat.PUFFIN) - .withPath("/tmp/dv.puffin") - .withFileSizeInBytes(10) - .withRecordCount(1) - .withReferencedDataFile("/tmp/data.parquet") - .withContentOffset(0L) - .withContentSizeInBytes(0L) - .build()) - .doesNotThrowAnyException(); + assertThatCode(() -> validDvBuilder().build()).doesNotThrowAnyException(); + } + + private static FileMetadata.Builder validDvBuilder() { + return FileMetadata.deleteFileBuilder(PartitionSpec.unpartitioned()) + .ofPositionDeletes() + .withFormat(FileFormat.PUFFIN) + .withPath("/tmp/dv.puffin") + .withFileSizeInBytes(10) + .withRecordCount(1) + .withReferencedDataFile("/tmp/data.parquet") + .withContentOffset(0L) + .withContentSizeInBytes(0L); } }