Core, Data: Validate deletion-vector offset and length by wombatu-kun · Pull Request #16500 · apache/iceberg

wombatu-kun · 2026-05-21T05:03:15Z

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 checks non-null and size <= Integer.MAX_VALUE, so a negative size slips through to new byte[length] and throws NegativeArraySizeException; a negative offset reaches IOUtil.readFully and produces an invalid seek. The same unguarded allocation pattern lives in DVUtil.readDV on the rewrite / DV-merge path used by mergeAndWriteDVsIfRequired.

Changes

Add DVUtil.validateDV as the canonical read-path validator (null offset, null size, offset >= 0, size >= 0, size < Integer.MAX_VALUE); DVUtil is now public so the data module can call it.
Have both BaseDeleteLoader.validateDV (data) and DVUtil.readDV (core) delegate to it. BaseDeleteLoader keeps its loader-specific referencedDataFile check.
For defense-in-depth, also reject negative contentOffset, negative contentSizeInBytes, and contentSizeInBytes >= Integer.MAX_VALUE in FileMetadata.Builder.build() for PUFFIN, mirroring the existing fileSizeInBytes >= 0 / recordCount >= 0 checks at the same site. 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

TestDVUtil (new) covers each precondition of the new validator: null offset/size, negative offset/size, the strict < Integer.MAX_VALUE boundary (rejected at Integer.MAX_VALUE, accepted at Integer.MAX_VALUE - 1), and valid zero / typical values.
TestFileMetadata (new) verifies the builder rejects negative DV offset, negative DV size, contentSizeInBytes >= Integer.MAX_VALUE, and still accepts valid values.
TestBaseDeleteLoader (new, in package org.apache.iceberg.data) uses a Mockito mock that bypasses the FileMetadata builder (which now rejects invalid values at construction), and asserts BaseDeleteLoader.loadPositionDeletes throws before any I/O is attempted.
Existing TestRowDelta (324 tests), TestDeletionVectorStruct, TestContentFileParser, TestDeleteFiles regression suites continue to pass.

Out of scope

PuffinReader / BlobMetadata similarly don't range-check blob offsets and lengths today, but Puffin is also used for non-DV blobs (statistics) and "valid" at that layer is a separate discussion of footer-authoritative vs. caller-supplied bounds. Leaving Puffin-level hardening as a follow-up.

AI Disclosure

Model: Claude Opus 4.7, Claude Opus 4.8
Platform/Tool: Claude Code
Human Oversight: fully reviewed
Prompt Summary: Validate the deletion-vector content_offset and content_size_in_bytes before using them.

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 apache#16475 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

rdblue · 2026-05-27T18:46:45Z

+    Preconditions.checkArgument(
+        dv.contentSizeInBytes() >= 0, "Invalid DV, length must be non-negative: %s", dvDesc(dv));
+    Preconditions.checkArgument(
+        dv.contentSizeInBytes() <= Integer.MAX_VALUE,


Doesn't this need to be strictly less than?

Fixed in 30256ab.

FWIW, probably something to add in the AGENTS.md code, no emdash, no non-ascii unicode except for UTF-related experiments. separate PR.

Opened #16653 to add the no-emdash / ASCII-only rule to AGENTS.md.

rdblue · 2026-05-27T18:57:31Z

        ContentFileUtil.isDV(deleteFile),
        "Cannot read, not a deletion vector: %s",
        deleteFile.location());
+    ContentFileUtil.validateDV(deleteFile);


Since this class already handles DeleteFile instances, I'd probably not modify ContentFileUtil and put the new method here. People aren't going to go looking for a DV validation method in ContentFileUtil.

Done in 30256ab. DVUtil is now public so BaseDeleteLoader (data module) can call it.

rdblue · 2026-05-27T19:01:03Z

+        Preconditions.checkArgument(
+            contentOffset >= 0, "Content offset must be non-negative for DV: %s", contentOffset);
+        Preconditions.checkArgument(
+            contentSizeInBytes >= 0,


Why not check < Integer.MAX_VALUE here as well?

Done in 30256ab.

rdblue · 2026-05-27T19:05:36Z

+  // 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) {


I think this is too clever and it's odd that the same PR has multiple ways to inject a delete with an invalid offset or size. I think this should pick one strategy. I think it probably makes more sense to use a mock, but I don't have a strong opinion.

Also, it would be reasonable (and straightforward) to just use the DeleteFile constructor and bypass the builder rather than doing things like this. The actual class allows these values because we don't validate and fail while filling the objects with data.

Done in 30256ab (Mockito mock).

rdblue · 2026-05-27T19:06:16Z

+    };
+  }
+
+  // The validator must fire before any I/O attempt — if it does not, the loader will call this


Please remove AI artifacts, like —

Done in 30256ab. Removed across the whole branch diff.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

steveloughran · 2026-06-01T19:25:07Z

+   * 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.
+   */


nit, add IllegalArgumentException to the javadocs

Done 00d7783.

steveloughran · 2026-06-01T19:28:57Z

+   * when the DV blob is read.
+   */
+  public static void validateDV(DeleteFile dv) {
+    Preconditions.checkArgument(


there's enough repetition here to make factoring stuff out something to consider, though I'll leave that to others to consider.
Everything looks for non-null, positive, and sizes must be < max_int.

split it out and you've got less to test

Left the five checks inline - each carries a distinct message (offset vs length, null vs negative vs 2GB) and they run through the public validateDV, so factoring wouldn't reduce the test surface. Happy to revisit if others prefer the split.

makes sense. the factoring of the construction works though, really makes clear what it is that you have set.

steveloughran · 2026-06-01T19:35:11Z

+                    .withReferencedDataFile("/tmp/data.parquet")
+                    .withContentOffset(0L)
+                    .withContentSizeInBytes(Integer.MAX_VALUE)
+                    .build())


given that all FileMetadata.Builder.Builder() setters are valid, you could factor these out to have a builder which creates a valid file, then each test overrides with the invalid value, e.g

assertThatThrownBy(() -> deleteFile() .withContentSizeInBytes(Integer.MAX_VALUE) .build())) ...

It'd make it clearer what is being invalid in each test.

again, personal opinion and I'll yield to others

Done 00d7783. Added a validDvBuilder() returning a valid builder; each test now overrides only the single invalid field.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions · 2026-07-03T00:37:51Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

wombatu-kun · 2026-07-03T02:37:23Z

no stale

steveloughran

LGTM, especially test coverage.

wombatu-kun · 2026-07-06T14:23:10Z

Hi @rdblue, gentle nudge on this one. All five of your comments from May 27 were addressed the next day in 30256ab67, and each thread has a reply pointing at the fix:

DV validation moved out of ContentFileUtil into a public DVUtil.validateDV, with the strict < Integer.MAX_VALUE bound you flagged
FileMetadata.Builder.build() now also rejects contentSizeInBytes >= Integer.MAX_VALUE
TestBaseDeleteLoader switched to a single Mockito-mock strategy
Em dashes removed across the diff

@steveloughran's later nits were handled in 00d778384, and he approved earlier today. Would you mind taking another pass when you have a moment? Thanks!

wombatu-kun · 2026-07-24T05:54:12Z

@rdblue friendly ping. All your review comments have been addressed (replies inline, plus commits 30256ab and 00d7783), and @steveloughran has approved. Could you take another look when you have a moment? Thanks!

steveloughran · 2026-07-24T13:10:58Z

I'd forgotten about this; once it is in the range check code of #17253 can go, leaving on the roaring bitmap parser logic.

steveloughran

commented; get this pr in and a slice of my hardening pr is surplus. Reviewing the tests, I've suggested factoring out some of the test code, as its somewhat repetitive. That's AI code generation for you: doesn't get bored and knows nothing of the extract-method operation of the IDE.
I think it's worth doing for future maintenance

steveloughran · 2026-07-24T13:15:32Z

+
+  @Test
+  public void validateDVRejectsNullOffset() {
+    DeleteFile dv = dv(null, 10L);


most of these tests can be refactored into an assertRejected(Long, Long, String) method, which is then invoked for all the values.

There's one step more advanced, which is to parameterize, though the way junit5 parameterizes it's more complicated to implement a method supplying a list of argument triples of (Long, Long, String) than it is to have the tests.

Done 36cec10. Took the parameterized variant for both the reject and accept cases, and applied the same shape to TestBaseDeleteLoader.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Generated-by: Claude Code (claude-opus-4-8)

github-actions Bot added core data labels May 21, 2026

rdblue reviewed May 27, 2026

View reviewed changes

Core, Data: Move DV validation to DVUtil and tighten size bound

30256ab

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

wombatu-kun requested a review from rdblue May 28, 2026 02:48

steveloughran reviewed Jun 1, 2026

View reviewed changes

Core: Document DV validation exception and dedupe DV builder tests

00d7783

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

wombatu-kun mentioned this pull request Jun 2, 2026

Docs: Add no-emdash / ASCII-only rule to AGENTS.md #16653

Open

wombatu-kun requested a review from steveloughran June 2, 2026 02:19

github-actions Bot added the stale label Jul 3, 2026

github-actions Bot removed the stale label Jul 4, 2026

steveloughran approved these changes Jul 6, 2026

View reviewed changes

steveloughran reviewed Jul 24, 2026

View reviewed changes

Core, Data: Parameterize DV validation tests

36cec10

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Generated-by: Claude Code (claude-opus-4-8)

Uh oh!

Conversation

wombatu-kun commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Out of scope

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 3, 2026

Uh oh!

wombatu-kun commented Jul 3, 2026

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

wombatu-kun commented Jul 6, 2026

Uh oh!

wombatu-kun commented Jul 24, 2026

Uh oh!

steveloughran commented Jul 24, 2026

Uh oh!

steveloughran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wombatu-kun commented May 21, 2026 •

edited

Loading