Skip to content

Commit 539aa14

Browse files
dfa1claude
andcommitted
fix(reader): reject a vortex.bool bitmap shorter than its row count
The gap #339 exposed one encoding over. MaterializedBoolArray#getBoolean indexes its buffer with no bounds check, and BoolEncodingDecoder handed it a file buffer straight through, so a truncated bitmap faulted with a raw IndexOutOfBoundsException on whichever row ran off the end — and `materialize` handed the short buffer to the caller intact. Checked once at decode, in O(1). The unchecked accessor stays unchecked on purpose: the other four construction sites all allocate the bitmap themselves at exactly (n + 7) / 8, so a per-row bound there would be dead at every site but this one, and this one can answer it once. The guard found a real under-sized fixture on its first run. RleEncodingEncoderTest's hand-built nullable-indices node supplied a 1-byte validity bitmap for an indices child that declares `indices_len` rows — which the encoder pads to a 1024 chunk boundary. Reading any row past 7 would have faulted; the test only ever read rows 0 through 3. The fixture now sizes the bitmap for what the node declares. Nothing else in the reactor, writer output or Rust fixture alike, supplied a short one. Companion to the #339 bytebool fix in this PR; same class of bug, and the bytebool PR is where it was spotted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent ddd406d commit 539aa14

5 files changed

Lines changed: 99 additions & 3 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- A `vortex.bytebool` column is now read in place from its mmapped buffer, as `docs/compatibility.md` already claimed: decode allocated an `n/8`-byte bitmap and ran a read-modify-write over every row to fill it, for the one boolean encoding whose buffer is already indexable per row. Callers that want a bitmap still get one from `materialize`. ([#339](https://github.com/dfa1/vortex-java/issues/339))
1313
- A `vortex.bytebool` buffer shorter than the declared row count now fails as `VortexException` instead of a raw `IndexOutOfBoundsException` on whichever row ran off the end. ([#339](https://github.com/dfa1/vortex-java/issues/339))
14+
- Same for a `vortex.bool` bitmap holding fewer than the `(rows + 7) / 8` bytes it needs, reached either as a column or as another column's validity child. ([#339](https://github.com/dfa1/vortex-java/issues/339))
1415

1516
- A malformed `fastlanes.delta` column no longer fails with a raw JDK exception: a row window running past the elements the chunks reconstruct threw `ArrayIndexOutOfBoundsException`, and an absurd or negative declared element count sized a heap array before anything checked it (`NegativeArraySizeException`, or `OutOfMemoryError`). All now fail as `VortexException`. ([#338](https://github.com/dfa1/vortex-java/issues/338))
1617
- A `fastlanes.delta` column no longer routes its decode through four row-scaled heap `long[]` arrays, every value widened to 8 bytes whatever the column's width; values are reconstructed into a single arena segment at the ptype's real width, and only the chunks overlapping the requested rows are reconstructed at all. ([#338](https://github.com/dfa1/vortex-java/issues/338))

reader/src/main/java/io/github/dfa1/vortex/reader/array/MaterializedBoolArray.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@
88

99
/// Buffer-backed [BoolArray] — the fallback used when an encoding decoder
1010
/// either materializes the output eagerly or has no lazy variant of its own.
11+
///
12+
/// [#getBoolean(long)] indexes `buffer` without a bounds check, so the buffer must hold at
13+
/// least `(length + 7) / 8` bytes. Every decoder that builds the bitmap itself allocates
14+
/// exactly that; the one that passes a file buffer through
15+
/// (`io.github.dfa1.vortex.reader.decode.BoolEncodingDecoder`) checks the size once before
16+
/// constructing this, rather than paying a bound per row.
1117
public final class MaterializedBoolArray extends AbstractMaterializedArray implements BoolArray {
1218

1319
/// Constructs a `MaterializedBoolArray` backed by the given bit-packed buffer.

reader/src/main/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoder.java

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
import io.github.dfa1.vortex.reader.array.MaskedArray;
99
import io.github.dfa1.vortex.reader.array.MaterializedBoolArray;
1010

11+
import java.lang.foreign.MemorySegment;
12+
1113
/// Read-only decoder for `vortex.bool` (bit-packed boolean arrays, LSB first).
1214
///
1315
/// When the encoding node has one child, that child is the validity bitmask:
@@ -23,7 +25,20 @@ public EncodingId encodingId() {
2325
@Override
2426
public Array decode(DecodeContext ctx) {
2527
long n = ctx.rowCount();
26-
Array values = new MaterializedBoolArray(ctx.dtype(), n, ctx.buffer(0));
28+
MemorySegment bits = ctx.buffer(0);
29+
// The bitmap comes straight from the file and needs one byte per 8 rows. A shorter one
30+
// is malformed, and [MaterializedBoolArray#getBoolean] indexes its buffer unchecked —
31+
// deliberately, since every other construction site allocates the bitmap itself at
32+
// exactly this size — so without this the read of whichever row runs off the end is a
33+
// raw IndexOutOfBoundsException (ADR 0003). O(1), and it also covers `materialize`,
34+
// which hands the same short buffer straight to the caller.
35+
long needed = (n + 7) >>> 3;
36+
if (bits.byteSize() < needed) {
37+
throw new VortexException(EncodingId.VORTEX_BOOL,
38+
"bool bitmap of " + bits.byteSize() + " byte(s) is shorter than the "
39+
+ needed + " byte(s) needed for " + n + " row(s)");
40+
}
41+
Array values = new MaterializedBoolArray(ctx.dtype(), n, bits);
2742
if (ctx.node().children().length == 1) {
2843
Array va = ctx.decodeChild(0, DType.BOOL, n);
2944
if (!(va instanceof BoolArray validity)) {

reader/src/test/java/io/github/dfa1/vortex/reader/decode/BoolEncodingDecoderTest.java

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package io.github.dfa1.vortex.reader.decode;
22

3+
import io.github.dfa1.vortex.core.error.VortexException;
34
import io.github.dfa1.vortex.core.model.EncodingId;
45
import io.github.dfa1.vortex.core.testing.DTypes;
56
import io.github.dfa1.vortex.reader.ReadRegistry;
@@ -9,13 +10,15 @@
910
import org.junit.jupiter.api.Test;
1011
import org.junit.jupiter.params.ParameterizedTest;
1112
import org.junit.jupiter.params.provider.Arguments;
13+
import org.junit.jupiter.params.provider.CsvSource;
1214
import org.junit.jupiter.params.provider.MethodSource;
1315

1416
import java.lang.foreign.Arena;
1517
import java.lang.foreign.MemorySegment;
1618
import java.util.stream.Stream;
1719

1820
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
1922

2023
class BoolEncodingDecoderTest {
2124

@@ -105,6 +108,73 @@ void decode_nullable_returnsMaskedArray_withNullsHidingUnderlying() {
105108
assertThat(inner.getBoolean(2)).isFalse();
106109
}
107110

111+
/// The bitmap is untrusted and needs one byte per 8 rows. `MaterializedBoolArray` indexes
112+
/// its buffer without a per-row bound — deliberately, since every other decoder that builds
113+
/// a bitmap allocates it at exactly the right size — so a short one used to fault as a raw
114+
/// `IndexOutOfBoundsException` on whichever row ran off the end, or hand the truncated
115+
/// buffer straight out of `materialize`. It must be a [VortexException] (ADR 0003).
116+
///
117+
/// The row counts are one past each byte boundary, where a bitmap is at its most
118+
/// deceptive: 9 rows need 2 bytes, not the 1 that covers the first 8.
119+
@ParameterizedTest
120+
@CsvSource({"9, 1", "17, 2", "65, 8", "8, 0"})
121+
void decode_bitmapShorterThanRowCount_throws(int rows, int suppliedBytes) {
122+
// Given
123+
MemorySegment bits = MemorySegment.ofArray(new byte[suppliedBytes]);
124+
ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{0});
125+
ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder());
126+
DecodeContext ctx = new DecodeContext(node, DTypes.BOOL, rows, new MemorySegment[]{bits},
127+
registry, Arena.ofAuto());
128+
var sut = new BoolEncodingDecoder();
129+
130+
// When / Then
131+
assertThatThrownBy(() -> sut.decode(ctx))
132+
.isInstanceOf(VortexException.class)
133+
.hasMessageContaining("bool bitmap");
134+
}
135+
136+
/// The same guard reached through the validity child, which decodes as its own
137+
/// `vortex.bool` array: a values bitmap long enough for the row count paired with a
138+
/// truncated validity bitmap must fail just as loudly.
139+
@Test
140+
void decode_nullable_validityBitmapTooShort_throws() {
141+
// Given — 9 rows of values (2 bytes) but a 1-byte validity bitmap
142+
MemorySegment bits = MemorySegment.ofArray(new byte[2]);
143+
MemorySegment validBits = MemorySegment.ofArray(new byte[1]);
144+
ArrayNode validityNode = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{1});
145+
ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[]{validityNode}, new int[]{0});
146+
ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder());
147+
DecodeContext ctx = new DecodeContext(node, DTypes.BOOL_N, 9,
148+
new MemorySegment[]{bits, validBits}, registry, Arena.ofAuto());
149+
var sut = new BoolEncodingDecoder();
150+
151+
// When / Then
152+
assertThatThrownBy(() -> sut.decode(ctx))
153+
.isInstanceOf(VortexException.class)
154+
.hasMessageContaining("bool bitmap");
155+
}
156+
157+
/// A bitmap longer than the row count needs is legal — trailing padding, or a buffer shared
158+
/// with a longer array — and must not be rejected by the size guard.
159+
@Test
160+
void decode_bitmapLongerThanNeeded_isAccepted() {
161+
// Given — 3 rows (1 byte needed) over an 8-byte buffer with bit 0 set
162+
MemorySegment bits = MemorySegment.ofArray(new byte[]{1, 0, 0, 0, 0, 0, 0, 0});
163+
ArrayNode node = new ArrayNode(EncodingId.VORTEX_BOOL, null, new ArrayNode[0], new int[]{0});
164+
ReadRegistry registry = TestRegistry.ofDecoders(new BoolEncodingDecoder());
165+
DecodeContext ctx = new DecodeContext(node, DTypes.BOOL, 3, new MemorySegment[]{bits},
166+
registry, Arena.ofAuto());
167+
var sut = new BoolEncodingDecoder();
168+
169+
// When
170+
var result = sut.decode(ctx);
171+
172+
// Then
173+
assertThat(result.length()).isEqualTo(3);
174+
assertThat(((BoolArray) result).getBoolean(0)).isTrue();
175+
assertThat(((BoolArray) result).getBoolean(1)).isFalse();
176+
}
177+
108178
@Test
109179
void decode_nullable_allNulls_allRowsInvalid() {
110180
// Given — every validity bit is false; values buffer content does not matter

writer/src/test/java/io/github/dfa1/vortex/writer/encode/RleEncodingEncoderTest.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -315,8 +315,12 @@ void decode_nullableIndices_returnsMaskedArrayWithCorrectValidity() {
315315
EncodeResult encoded = ENCODER.encode(dtype, data, EncodeTestHelper.testCtx());
316316

317317
List<MemorySegment> originalBufs = new ArrayList<>(encoded.buffers());
318-
MemorySegment validityBuf = MemorySegment.ofArray(new byte[]{0x05});
319-
originalBufs.add(validityBuf);
318+
// The indices child declares `indices_len` rows, which the encoder pads to a 1024
319+
// chunk boundary, so its validity bitmap has to cover all 1024 — not just the four
320+
// rows this fixture cares about. Bits 0 and 2 set: rows 0 and 2 valid, 1 and 3 null.
321+
byte[] validityBits = new byte[(1024 + 7) / 8];
322+
validityBits[0] = 0x05;
323+
originalBufs.add(MemorySegment.ofArray(validityBits));
320324
MemorySegment[] segments = originalBufs.toArray(new MemorySegment[0]);
321325

322326
ArrayNode origRoot = toArrayNode(encoded.rootNode());

0 commit comments

Comments
 (0)