From 5892f8f1c0d6e2ed9338c2d778e9076ee42e98a3 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Sat, 31 Oct 2026 07:18:10 -0600 Subject: [PATCH 1/2] fix(vector): validate decimal value length before raw writes The byte[] and ArrowBuf variants of setBigEndian/setBigEndianSafe and setSafe(int, long, ArrowBuf, int) in DecimalVector and Decimal256Vector copy directly into native memory with MemoryUtil. Check that the value length fits in the TYPE_WIDTH slot before any write, and treat a zero-length ArrowBuf value as zero instead of sign-extending from the byte before start. Closes #1417. --- .../apache/arrow/vector/Decimal256Vector.java | 46 +++++++---- .../apache/arrow/vector/DecimalVector.java | 45 +++++++---- .../arrow/vector/TestDecimal256Vector.java | 77 ++++++++++++++++++ .../arrow/vector/TestDecimalVector.java | 80 +++++++++++++++++++ 4 files changed, 215 insertions(+), 33 deletions(-) diff --git a/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java b/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java index f9d7e5cb9e..61ec8269f9 100644 --- a/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java +++ b/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java @@ -205,8 +205,9 @@ public void set(int index, ArrowBuf buffer) { * @param value array of bytes containing decimal in big endian byte order. */ public void setBigEndian(int index, byte[] value) { - BitVectorHelper.setBit(validityBuffer, index); final int length = value.length; + checkValueLength(length); + BitVectorHelper.setBit(validityBuffer, index); // do the bound check. valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH); @@ -222,29 +223,30 @@ public void setBigEndian(int index, byte[] value) { MemoryUtil.putByte(outAddress + byteIdx, value[length - 1 - byteIdx]); } - if (length == TYPE_WIDTH) { - return; - } - if (length < TYPE_WIDTH) { // sign extend final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); MemoryUtil.setMemory(outAddress + length, Decimal256Vector.TYPE_WIDTH - length, pad); - return; } } else { - if (length <= TYPE_WIDTH) { - // copy data from value to outAddress - MemoryUtil.copyToMemory( - value, 0, outAddress + Decimal256Vector.TYPE_WIDTH - length, length); - // sign extend - final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); - MemoryUtil.setMemory(outAddress, Decimal256Vector.TYPE_WIDTH - length, pad); - return; - } + // copy data from value to outAddress + MemoryUtil.copyToMemory(value, 0, outAddress + Decimal256Vector.TYPE_WIDTH - length, length); + // sign extend + final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); + MemoryUtil.setMemory(outAddress, Decimal256Vector.TYPE_WIDTH - length, pad); + } + } + + /** + * Validates the byte length of a raw decimal value before it is written. The raw write paths + * copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length + * larger than {@link #TYPE_WIDTH} would overrun the value slot. + */ + private static void checkValueLength(int length) { + if (length < 0 || length > TYPE_WIDTH) { + throw new IllegalArgumentException( + "Invalid decimal value length. Valid length in [1 - 32], got " + length); } - throw new IllegalArgumentException( - "Invalid decimal value length. Valid length in [1 - 32], got " + length); } /** @@ -268,6 +270,7 @@ public void set(int index, long start, ArrowBuf buffer) { * @param length length of the value in the buffer */ public void setSafe(int index, long start, ArrowBuf buffer, int length) { + checkValueLength(length); handleSafe(index); BitVectorHelper.setBit(validityBuffer, index); @@ -277,6 +280,10 @@ public void setSafe(int index, long start, ArrowBuf buffer, int length) { long inAddress = buffer.memoryAddress() + start; long outAddress = valueBuffer.memoryAddress() + (long) index * TYPE_WIDTH; + if (length == 0) { + MemoryUtil.setMemory(outAddress, Decimal256Vector.TYPE_WIDTH, (byte) 0); + return; + } if (LITTLE_ENDIAN) { MemoryUtil.copyMemory(inAddress, outAddress, length); // sign extend @@ -305,6 +312,7 @@ public void setSafe(int index, long start, ArrowBuf buffer, int length) { * @param length length of the value in the buffer */ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length) { + checkValueLength(length); handleSafe(index); BitVectorHelper.setBit(validityBuffer, index); @@ -315,6 +323,10 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length) // not using buffer.getByte() to avoid boundary checks for every byte. long inAddress = buffer.memoryAddress() + start; long outAddress = valueBuffer.memoryAddress() + (long) index * TYPE_WIDTH; + if (length == 0) { + MemoryUtil.setMemory(outAddress, Decimal256Vector.TYPE_WIDTH, (byte) 0); + return; + } if (LITTLE_ENDIAN) { // swap bytes to convert BE to LE for (int byteIdx = 0; byteIdx < length; ++byteIdx) { diff --git a/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java b/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java index 9bf1812cc6..ba36a4cd81 100644 --- a/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java @@ -204,8 +204,9 @@ public void set(int index, ArrowBuf buffer) { * @param value array of bytes containing decimal in big endian byte order. */ public void setBigEndian(int index, byte[] value) { - BitVectorHelper.setBit(validityBuffer, index); final int length = value.length; + checkValueLength(length); + BitVectorHelper.setBit(validityBuffer, index); // do the bound check. valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH); @@ -221,28 +222,30 @@ public void setBigEndian(int index, byte[] value) { MemoryUtil.putByte(outAddress + byteIdx, value[length - 1 - byteIdx]); } - if (length == TYPE_WIDTH) { - return; - } - if (length < TYPE_WIDTH) { // sign extend final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); MemoryUtil.setMemory(outAddress + length, DecimalVector.TYPE_WIDTH - length, pad); - return; } } else { - if (length <= TYPE_WIDTH) { - // copy data from value to outAddress - MemoryUtil.copyToMemory(value, 0, outAddress + DecimalVector.TYPE_WIDTH - length, length); - // sign extend - final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); - MemoryUtil.setMemory(outAddress, DecimalVector.TYPE_WIDTH - length, pad); - return; - } + // copy data from value to outAddress + MemoryUtil.copyToMemory(value, 0, outAddress + DecimalVector.TYPE_WIDTH - length, length); + // sign extend + final byte pad = (byte) (value[0] < 0 ? 0xFF : 0x00); + MemoryUtil.setMemory(outAddress, DecimalVector.TYPE_WIDTH - length, pad); + } + } + + /** + * Validates the byte length of a raw decimal value before it is written. The raw write paths + * copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length + * larger than {@link #TYPE_WIDTH} would overrun the value slot. + */ + private static void checkValueLength(int length) { + if (length < 0 || length > TYPE_WIDTH) { + throw new IllegalArgumentException( + "Invalid decimal value length. Valid length in [1 - 16], got " + length); } - throw new IllegalArgumentException( - "Invalid decimal value length. Valid length in [1 - 16], got " + length); } /** @@ -266,6 +269,7 @@ public void set(int index, long start, ArrowBuf buffer) { * @param length length of the value in the buffer */ public void setSafe(int index, long start, ArrowBuf buffer, int length) { + checkValueLength(length); handleSafe(index); BitVectorHelper.setBit(validityBuffer, index); @@ -275,6 +279,10 @@ public void setSafe(int index, long start, ArrowBuf buffer, int length) { long inAddress = buffer.memoryAddress() + start; long outAddress = valueBuffer.memoryAddress() + (long) index * TYPE_WIDTH; + if (length == 0) { + MemoryUtil.setMemory(outAddress, DecimalVector.TYPE_WIDTH, (byte) 0); + return; + } if (LITTLE_ENDIAN) { MemoryUtil.copyMemory(inAddress, outAddress, length); // sign extend @@ -303,6 +311,7 @@ public void setSafe(int index, long start, ArrowBuf buffer, int length) { * @param length length of the value in the buffer */ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length) { + checkValueLength(length); handleSafe(index); BitVectorHelper.setBit(validityBuffer, index); @@ -313,6 +322,10 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length) // not using buffer.getByte() to avoid boundary checks for every byte. long inAddress = buffer.memoryAddress() + start; long outAddress = valueBuffer.memoryAddress() + (long) index * TYPE_WIDTH; + if (length == 0) { + MemoryUtil.setMemory(outAddress, DecimalVector.TYPE_WIDTH, (byte) 0); + return; + } if (LITTLE_ENDIAN) { // swap bytes to convert BE to LE for (int byteIdx = 0; byteIdx < length; ++byteIdx) { diff --git a/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java b/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java index b995dc5d92..71a03250a2 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java @@ -17,6 +17,7 @@ package org.apache.arrow.vector; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -419,4 +420,80 @@ private void verifyWritingArrowBufWithBigEndianBytes( assertEquals(expectedValues[i], value); } } + + // ---- Oversized raw writes ------------------------------------------------------------------- + // The value buffer and validity buffer are adjacent slices of one allocation (value buffer first), + // so a write that overruns the last value slot lands in the validity buffer. That makes an overrun + // observable from Java via isNull() without touching memory outside the allocation. Each test + // marks the first 8 values valid, attempts an oversized write into the last slot whose spilled + // byte is zero, and asserts the write is rejected without clearing any validity bit. + + private static final int OVERSIZED_LENGTH = Decimal256Vector.TYPE_WIDTH + 1; + + private Decimal256Vector newVectorWithFirstEightValid() { + Decimal256Vector vector = new Decimal256Vector("decimal", allocator, 38, 0); + vector.allocateNew(64); + for (int i = 0; i < 8; i++) { + vector.setSafe(i, BigDecimal.valueOf(i)); + } + vector.setValueCount(8); + return vector; + } + + private static void assertFirstEightValid(Decimal256Vector vector) { + for (int i = 0; i < 8; i++) { + assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write"); + assertEquals(BigDecimal.valueOf(i), vector.getObject(i)); + } + } + + @Test + public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() { + try (Decimal256Vector vector = newVectorWithFirstEightValid()) { + final int last = vector.getValueCapacity() - 1; + final byte[] oversized = new byte[OVERSIZED_LENGTH]; + assertThrows( + IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized)); + assertFirstEightValid(vector); + } + } + + @Test + public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + try (Decimal256Vector vector = newVectorWithFirstEightValid(); + ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { + src.setZero(0, OVERSIZED_LENGTH); + final int last = vector.getValueCapacity() - 1; + assertThrows( + IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH)); + assertFirstEightValid(vector); + } + } + + @Test + public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + try (Decimal256Vector vector = newVectorWithFirstEightValid(); + ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { + src.setZero(0, OVERSIZED_LENGTH); + final int last = vector.getValueCapacity() - 1; + assertThrows( + IllegalArgumentException.class, + () -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH)); + assertFirstEightValid(vector); + } + } + + @Test + public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() { + try (Decimal256Vector vector = new Decimal256Vector("decimal", allocator, 38, 0); + ArrowBuf src = allocator.buffer(2)) { + vector.allocateNew(1); + // A negative byte immediately before `start` must not be used for sign extension. + src.setByte(0, (byte) 0xFF); + src.setByte(1, (byte) 0x00); + vector.setSafe(0, 1, src, 0); + vector.setValueCount(1); + assertEquals(BigDecimal.ZERO, vector.getObject(0)); + } + } } diff --git a/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java b/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java index 85c11e8f3d..60ed1a73bc 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java @@ -17,6 +17,8 @@ package org.apache.arrow.vector; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -413,4 +415,82 @@ private void verifyWritingArrowBufWithBigEndianBytes( assertEquals(expectedValues[i], value); } } + + // ---- Oversized raw writes ------------------------------------------------------------------- + // The value buffer and validity buffer are adjacent slices of one allocation (value buffer first), + // so a write that overruns the last value slot lands in the validity buffer. That makes an overrun + // observable from Java via isNull() without touching memory outside the allocation. Each test + // marks the first 8 values valid, attempts an oversized write into the last slot whose spilled + // byte is zero, and asserts the write is rejected without clearing any validity bit. + + private static final int OVERSIZED_LENGTH = DecimalVector.TYPE_WIDTH + 1; + + private DecimalVector newVectorWithFirstEightValid() { + DecimalVector vector = TestUtils.newVector( + DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator); + vector.allocateNew(64); + for (int i = 0; i < 8; i++) { + vector.setSafe(i, BigDecimal.valueOf(i)); + } + vector.setValueCount(8); + return vector; + } + + private static void assertFirstEightValid(DecimalVector vector) { + for (int i = 0; i < 8; i++) { + assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write"); + assertEquals(BigDecimal.valueOf(i), vector.getObject(i)); + } + } + + @Test + public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() { + try (DecimalVector vector = newVectorWithFirstEightValid()) { + final int last = vector.getValueCapacity() - 1; + final byte[] oversized = new byte[OVERSIZED_LENGTH]; + assertThrows( + IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized)); + assertFirstEightValid(vector); + } + } + + @Test + public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + try (DecimalVector vector = newVectorWithFirstEightValid(); + ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { + src.setZero(0, OVERSIZED_LENGTH); + final int last = vector.getValueCapacity() - 1; + assertThrows( + IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH)); + assertFirstEightValid(vector); + } + } + + @Test + public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + try (DecimalVector vector = newVectorWithFirstEightValid(); + ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { + src.setZero(0, OVERSIZED_LENGTH); + final int last = vector.getValueCapacity() - 1; + assertThrows( + IllegalArgumentException.class, + () -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH)); + assertFirstEightValid(vector); + } + } + + @Test + public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() { + try (DecimalVector vector = TestUtils.newVector( + DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator); + ArrowBuf src = allocator.buffer(2)) { + vector.allocateNew(1); + // A negative byte immediately before `start` must not be used for sign extension. + src.setByte(0, (byte) 0xFF); + src.setByte(1, (byte) 0x00); + vector.setSafe(0, 1, src, 0); + vector.setValueCount(1); + assertEquals(BigDecimal.ZERO, vector.getObject(0)); + } + } } From 67cd552b8723e20cfc52ce8f8ac386e75fe940db Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Sat, 31 Oct 2026 07:29:45 -0600 Subject: [PATCH 2/2] refactor(vector): tidy decimal length validation and tests Use Preconditions.checkArgument with TYPE_WIDTH in the message, replace a stray 32 literal, collapse the oversized-write tests into one helper, add a zero-length test for setBigEndianSafe(ArrowBuf), and shorten comments so they fit the 100 column limit enforced by spotless. --- .../apache/arrow/vector/Decimal256Vector.java | 17 ++-- .../apache/arrow/vector/DecimalVector.java | 15 ++-- .../arrow/vector/TestDecimal256Vector.java | 70 ++++++++--------- .../arrow/vector/TestDecimalVector.java | 78 +++++++++---------- 4 files changed, 84 insertions(+), 96 deletions(-) diff --git a/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java b/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java index 61ec8269f9..ddfc464152 100644 --- a/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java +++ b/vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java @@ -23,6 +23,7 @@ import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.BufferAllocator; import org.apache.arrow.memory.util.MemoryUtil; +import org.apache.arrow.util.Preconditions; import org.apache.arrow.vector.complex.impl.Decimal256ReaderImpl; import org.apache.arrow.vector.complex.reader.FieldReader; import org.apache.arrow.vector.holders.Decimal256Holder; @@ -237,16 +238,12 @@ public void setBigEndian(int index, byte[] value) { } } - /** - * Validates the byte length of a raw decimal value before it is written. The raw write paths - * copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length - * larger than {@link #TYPE_WIDTH} would overrun the value slot. - */ private static void checkValueLength(int length) { - if (length < 0 || length > TYPE_WIDTH) { - throw new IllegalArgumentException( - "Invalid decimal value length. Valid length in [1 - 32], got " + length); - } + Preconditions.checkArgument( + length >= 0 && length <= TYPE_WIDTH, + "Invalid decimal value length. Valid length in [0 - %s], got %s", + TYPE_WIDTH, + length); } /** @@ -334,7 +331,7 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length) MemoryUtil.putByte(outAddress + byteIdx, val); } // sign extend - if (length < 32) { + if (length < TYPE_WIDTH) { byte msb = MemoryUtil.getByte(inAddress); final byte pad = (byte) (msb < 0 ? 0xFF : 0x00); MemoryUtil.setMemory(outAddress + length, Decimal256Vector.TYPE_WIDTH - length, pad); diff --git a/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java b/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java index ba36a4cd81..927fe94fa1 100644 --- a/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/DecimalVector.java @@ -23,6 +23,7 @@ import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.BufferAllocator; import org.apache.arrow.memory.util.MemoryUtil; +import org.apache.arrow.util.Preconditions; import org.apache.arrow.vector.complex.impl.DecimalReaderImpl; import org.apache.arrow.vector.complex.reader.FieldReader; import org.apache.arrow.vector.holders.DecimalHolder; @@ -236,16 +237,12 @@ public void setBigEndian(int index, byte[] value) { } } - /** - * Validates the byte length of a raw decimal value before it is written. The raw write paths - * copy straight into native memory and bypass {@link ArrowBuf} bounds checking, so a length - * larger than {@link #TYPE_WIDTH} would overrun the value slot. - */ private static void checkValueLength(int length) { - if (length < 0 || length > TYPE_WIDTH) { - throw new IllegalArgumentException( - "Invalid decimal value length. Valid length in [1 - 16], got " + length); - } + Preconditions.checkArgument( + length >= 0 && length <= TYPE_WIDTH, + "Invalid decimal value length. Valid length in [0 - %s], got %s", + TYPE_WIDTH, + length); } /** diff --git a/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java b/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java index 71a03250a2..7de109f1e5 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java @@ -24,6 +24,7 @@ import java.math.BigDecimal; import java.math.BigInteger; +import java.util.function.BiConsumer; import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.BufferAllocator; import org.apache.arrow.vector.types.pojo.ArrowType; @@ -269,7 +270,7 @@ public void decimalBE2NE() { IllegalArgumentException.class, () -> decimalVector.setBigEndian(insertionIdxCapture, new byte[33])); assertTrue( - ex.getMessage().equals("Invalid decimal value length. Valid length in [1 - 32], got 33")); + ex.getMessage().equals("Invalid decimal value length. Valid length in [0 - 32], got 33")); decimalVector.setValueCount(insertionIdx); // retrieve values and check if they are correct @@ -421,13 +422,6 @@ private void verifyWritingArrowBufWithBigEndianBytes( } } - // ---- Oversized raw writes ------------------------------------------------------------------- - // The value buffer and validity buffer are adjacent slices of one allocation (value buffer first), - // so a write that overruns the last value slot lands in the validity buffer. That makes an overrun - // observable from Java via isNull() without touching memory outside the allocation. Each test - // marks the first 8 values valid, attempts an oversized write into the last slot whose spilled - // byte is zero, and asserts the write is rejected without clearing any validity bit. - private static final int OVERSIZED_LENGTH = Decimal256Vector.TYPE_WIDTH + 1; private Decimal256Vector newVectorWithFirstEightValid() { @@ -436,10 +430,10 @@ private Decimal256Vector newVectorWithFirstEightValid() { for (int i = 0; i < 8; i++) { vector.setSafe(i, BigDecimal.valueOf(i)); } - vector.setValueCount(8); return vector; } + // Validity bits sit right after the value buffer, so an overrun of the last slot clobbers them. private static void assertFirstEightValid(Decimal256Vector vector) { for (int i = 0; i < 8; i++) { assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write"); @@ -447,40 +441,33 @@ private static void assertFirstEightValid(Decimal256Vector vector) { } } - @Test - public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() { - try (Decimal256Vector vector = newVectorWithFirstEightValid()) { - final int last = vector.getValueCapacity() - 1; - final byte[] oversized = new byte[OVERSIZED_LENGTH]; - assertThrows( - IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized)); - assertFirstEightValid(vector); - } - } - - @Test - public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + private void assertOversizedWriteRejected(BiConsumer write) { try (Decimal256Vector vector = newVectorWithFirstEightValid(); ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { src.setZero(0, OVERSIZED_LENGTH); - final int last = vector.getValueCapacity() - 1; - assertThrows( - IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH)); + assertThrows(IllegalArgumentException.class, () -> write.accept(vector, src)); assertFirstEightValid(vector); } } @Test - public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { - try (Decimal256Vector vector = newVectorWithFirstEightValid(); - ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { - src.setZero(0, OVERSIZED_LENGTH); - final int last = vector.getValueCapacity() - 1; - assertThrows( - IllegalArgumentException.class, - () -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH)); - assertFirstEightValid(vector); - } + public void setBigEndianSafeRejectsOversizedByteArray() { + assertOversizedWriteRejected( + (vector, src) -> + vector.setBigEndianSafe(vector.getValueCapacity() - 1, new byte[OVERSIZED_LENGTH])); + } + + @Test + public void setSafeFromArrowBufRejectsOversizedLength() { + assertOversizedWriteRejected( + (vector, src) -> vector.setSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH)); + } + + @Test + public void setBigEndianSafeFromArrowBufRejectsOversizedLength() { + assertOversizedWriteRejected( + (vector, src) -> + vector.setBigEndianSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH)); } @Test @@ -492,7 +479,18 @@ public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() { src.setByte(0, (byte) 0xFF); src.setByte(1, (byte) 0x00); vector.setSafe(0, 1, src, 0); - vector.setValueCount(1); + assertEquals(BigDecimal.ZERO, vector.getObject(0)); + } + } + + @Test + public void setBigEndianSafeFromArrowBufWithZeroLengthDoesNotReadAtStart() { + try (Decimal256Vector vector = new Decimal256Vector("decimal", allocator, 38, 0); + ArrowBuf src = allocator.buffer(1)) { + vector.allocateNew(1); + // A negative byte at `start` must not be used for sign extension of an empty value. + src.setByte(0, (byte) 0xFF); + vector.setBigEndianSafe(0, 0, src, 0); assertEquals(BigDecimal.ZERO, vector.getObject(0)); } } diff --git a/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java b/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java index 60ed1a73bc..cca1bfe854 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java @@ -17,14 +17,15 @@ package org.apache.arrow.vector; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.function.BiConsumer; import org.apache.arrow.memory.ArrowBuf; import org.apache.arrow.memory.BufferAllocator; import org.apache.arrow.vector.types.pojo.ArrowType; @@ -265,7 +266,7 @@ public void decimalBE2NE() { } catch (IllegalArgumentException ex) { assertTrue( ex.getMessage() - .equals("Invalid decimal value length. Valid length in [1 - 16], got 17")); + .equals("Invalid decimal value length. Valid length in [0 - 16], got 17")); } decimalVector.setValueCount(insertionIdx); @@ -416,26 +417,18 @@ private void verifyWritingArrowBufWithBigEndianBytes( } } - // ---- Oversized raw writes ------------------------------------------------------------------- - // The value buffer and validity buffer are adjacent slices of one allocation (value buffer first), - // so a write that overruns the last value slot lands in the validity buffer. That makes an overrun - // observable from Java via isNull() without touching memory outside the allocation. Each test - // marks the first 8 values valid, attempts an oversized write into the last slot whose spilled - // byte is zero, and asserts the write is rejected without clearing any validity bit. - private static final int OVERSIZED_LENGTH = DecimalVector.TYPE_WIDTH + 1; private DecimalVector newVectorWithFirstEightValid() { - DecimalVector vector = TestUtils.newVector( - DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator); + DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0); vector.allocateNew(64); for (int i = 0; i < 8; i++) { vector.setSafe(i, BigDecimal.valueOf(i)); } - vector.setValueCount(8); return vector; } + // Validity bits sit right after the value buffer, so an overrun of the last slot clobbers them. private static void assertFirstEightValid(DecimalVector vector) { for (int i = 0; i < 8; i++) { assertFalse(vector.isNull(i), "validity bit " + i + " was overwritten by an oversized write"); @@ -443,53 +436,56 @@ private static void assertFirstEightValid(DecimalVector vector) { } } - @Test - public void setBigEndianSafeRejectsOversizedByteArrayWithoutWriting() { - try (DecimalVector vector = newVectorWithFirstEightValid()) { - final int last = vector.getValueCapacity() - 1; - final byte[] oversized = new byte[OVERSIZED_LENGTH]; - assertThrows( - IllegalArgumentException.class, () -> vector.setBigEndianSafe(last, oversized)); - assertFirstEightValid(vector); - } - } - - @Test - public void setSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { + private void assertOversizedWriteRejected(BiConsumer write) { try (DecimalVector vector = newVectorWithFirstEightValid(); ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { src.setZero(0, OVERSIZED_LENGTH); - final int last = vector.getValueCapacity() - 1; - assertThrows( - IllegalArgumentException.class, () -> vector.setSafe(last, 0, src, OVERSIZED_LENGTH)); + assertThrows(IllegalArgumentException.class, () -> write.accept(vector, src)); assertFirstEightValid(vector); } } @Test - public void setBigEndianSafeFromArrowBufRejectsOversizedLengthWithoutWriting() { - try (DecimalVector vector = newVectorWithFirstEightValid(); - ArrowBuf src = allocator.buffer(OVERSIZED_LENGTH)) { - src.setZero(0, OVERSIZED_LENGTH); - final int last = vector.getValueCapacity() - 1; - assertThrows( - IllegalArgumentException.class, - () -> vector.setBigEndianSafe(last, 0, src, OVERSIZED_LENGTH)); - assertFirstEightValid(vector); - } + public void setBigEndianSafeRejectsOversizedByteArray() { + assertOversizedWriteRejected( + (vector, src) -> + vector.setBigEndianSafe(vector.getValueCapacity() - 1, new byte[OVERSIZED_LENGTH])); + } + + @Test + public void setSafeFromArrowBufRejectsOversizedLength() { + assertOversizedWriteRejected( + (vector, src) -> vector.setSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH)); + } + + @Test + public void setBigEndianSafeFromArrowBufRejectsOversizedLength() { + assertOversizedWriteRejected( + (vector, src) -> + vector.setBigEndianSafe(vector.getValueCapacity() - 1, 0, src, OVERSIZED_LENGTH)); } @Test public void setSafeFromArrowBufWithZeroLengthDoesNotReadBeforeStart() { - try (DecimalVector vector = TestUtils.newVector( - DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator); + try (DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0); ArrowBuf src = allocator.buffer(2)) { vector.allocateNew(1); // A negative byte immediately before `start` must not be used for sign extension. src.setByte(0, (byte) 0xFF); src.setByte(1, (byte) 0x00); vector.setSafe(0, 1, src, 0); - vector.setValueCount(1); + assertEquals(BigDecimal.ZERO, vector.getObject(0)); + } + } + + @Test + public void setBigEndianSafeFromArrowBufWithZeroLengthDoesNotReadAtStart() { + try (DecimalVector vector = new DecimalVector("decimal", allocator, 38, 0); + ArrowBuf src = allocator.buffer(1)) { + vector.allocateNew(1); + // A negative byte at `start` must not be used for sign extension of an empty value. + src.setByte(0, (byte) 0xFF); + vector.setBigEndianSafe(0, 0, src, 0); assertEquals(BigDecimal.ZERO, vector.getObject(0)); } }