From 4d35facb83ae0b2416b1ddd75a26cf7ef681987e Mon Sep 17 00:00:00 2001 From: David Li Date: Thu, 10 Sep 2026 15:08:02 +0900 Subject: [PATCH 1/6] GH-XXXX: Defer byte[] allocation until after bounds check Also, document that when enable_unsafe_memory_access is enabled, all bets are off. Reported by n0mi1k. --- .../org/apache/arrow/memory/ArrowBuf.java | 21 +++++++++++++++++-- .../apache/arrow/memory/BoundsChecking.java | 4 ++++ .../vector/BaseVariableWidthViewVector.java | 10 ++++----- .../vector/TestVariableWidthViewVector.java | 16 ++++++++++++++ 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index 9712be34d7..c34e99df23 100644 --- a/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -731,6 +731,24 @@ public void getBytes(long index, byte[] dst, int dstIndex, int length) { } } + /** + * Copy data from this ArrowBuf into a newly allocated array. + * + *

This method is more resilient to invalid data inadvertently causing large allocations, + * as the byte[] will not be allocated until we check the length. + * + * @param index index (0 based relative to the portion of memory this ArrowBuf has access to) + * @param length length of data to copy from this ArrowBuf + */ + public byte[] getBytesAsArray(long index, int length) { + checkIndex(index, length); + byte[] dst = new byte[length]; + if (length != 0) { + MemoryUtil.copyFromMemory(addr(index), dst, 0, length); + } + return dst; + } + /** * Copy data from a given byte array into this ArrowBuf starting at a given index. * @@ -1008,8 +1026,7 @@ public int setBytes(long index, InputStream in, int length) throws IOException { /** * Copy a certain length of bytes from this ArrowBuf at a given index into the given OutputStream. * - * @param index index index (0 based relative to the portion of memory this ArrowBuf has access - * to) + * @param index index (0 based relative to the portion of memory this ArrowBuf has access to) * @param out dst stream to copy data into * @param length length of data to copy * @throws IOException on failing to write to stream diff --git a/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java b/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java index 50be9ad1fb..ed5a2644e3 100644 --- a/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java +++ b/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java @@ -24,6 +24,10 @@ * "arrow.enable_unsafe_memory_access" or "drill.enable_unsafe_memory_access". The latter is * deprecated. The environmental variable is named "ARROW_ENABLE_UNSAFE_MEMORY_ACCESS". When both * the system property and the environmental variable are set, the system property takes precedence. + * + *

WARNING: disabling bounds checking means that out-of-bounds memory access is possible! This can + * lead to security vulnerabilities. You should not read or write untrusted data when bounds checking + * is disabled. */ public class BoundsChecking { diff --git a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java index 8d2a2d7401..da503f95b5 100644 --- a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java @@ -1556,7 +1556,6 @@ public int hashCode(int index, ArrowBufHasher hasher) { */ protected byte[] getData(int index) { final int dataLength = getValueLength(index); - byte[] result = new byte[dataLength]; if (dataLength > INLINE_SIZE) { // data is in the data buffer // get buffer index @@ -1566,12 +1565,11 @@ protected byte[] getData(int index) { final int dataOffset = viewBuffer.getInt( ((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); - dataBuffers.get(bufferIndex).getBytes(dataOffset, result, 0, dataLength); - } else { - // data is in the view buffer - viewBuffer.getBytes((long) index * ELEMENT_SIZE + BUF_INDEX_WIDTH, result, 0, dataLength); + ArrowBuf dataBuffer = dataBuffers.get(bufferIndex); + return dataBuffer.getBytesAsArray(dataOffset, dataLength); } - return result; + // data is in the view buffer + return viewBuffer.getBytesAsArray((long) index * ELEMENT_SIZE + BUF_INDEX_WIDTH, dataLength); } protected void getData(int index, ReusableBuffer buffer) { diff --git a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java index c4a1ae9b61..cdd687f4e5 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java @@ -2921,4 +2921,20 @@ public void testValidate() { assertTrue(e.getMessage().contains("Not enough capacity for data buffer")); } } + + @Test + public void testValidateInvalidOffsets() { + try (final ViewVarCharVector vector = new ViewVarCharVector("v", allocator)) { + vector.allocateNew(8, 1); + vector.allocateOrGetLastDataBuffer(8); + var offsets = vector.getDataBuffer(); + offsets.setInt(0, 64); + offsets.setInt(1, 0); + offsets.setInt(2, 0); + offsets.setInt(3, 1024); + vector.setValueCount(1); + vector.setIndexDefined(0); + assertThrows(IndexOutOfBoundsException.class, vector::validateFull); + } + } } From 01e3fef1d8ae380f30953652fde71e7afafcb9cb Mon Sep 17 00:00:00 2001 From: David Li Date: Thu, 10 Sep 2026 15:38:37 +0900 Subject: [PATCH 2/6] spotless --- .../src/main/java/org/apache/arrow/memory/ArrowBuf.java | 4 ++-- .../main/java/org/apache/arrow/memory/BoundsChecking.java | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java b/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java index c34e99df23..c3eafaacf3 100644 --- a/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java +++ b/memory/memory-core/src/main/java/org/apache/arrow/memory/ArrowBuf.java @@ -734,8 +734,8 @@ public void getBytes(long index, byte[] dst, int dstIndex, int length) { /** * Copy data from this ArrowBuf into a newly allocated array. * - *

This method is more resilient to invalid data inadvertently causing large allocations, - * as the byte[] will not be allocated until we check the length. + *

This method is more resilient to invalid data inadvertently causing large allocations, as + * the byte[] will not be allocated until we check the length. * * @param index index (0 based relative to the portion of memory this ArrowBuf has access to) * @param length length of data to copy from this ArrowBuf diff --git a/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java b/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java index ed5a2644e3..9b99edafa4 100644 --- a/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java +++ b/memory/memory-core/src/main/java/org/apache/arrow/memory/BoundsChecking.java @@ -25,9 +25,9 @@ * deprecated. The environmental variable is named "ARROW_ENABLE_UNSAFE_MEMORY_ACCESS". When both * the system property and the environmental variable are set, the system property takes precedence. * - *

WARNING: disabling bounds checking means that out-of-bounds memory access is possible! This can - * lead to security vulnerabilities. You should not read or write untrusted data when bounds checking - * is disabled. + *

WARNING: disabling bounds checking means that out-of-bounds memory access is possible! This + * can lead to security vulnerabilities. You should not read or write untrusted data when bounds + * checking is disabled. */ public class BoundsChecking { From 44781895892c5f23f694889fd273837f023d5d14 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 11 Sep 2026 12:06:31 +0900 Subject: [PATCH 3/6] consolidate checks --- .../vector/BaseVariableWidthViewVector.java | 144 +++++++++--------- .../vector/TestVariableWidthViewVector.java | 11 +- 2 files changed, 74 insertions(+), 81 deletions(-) diff --git a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java index da503f95b5..8760f34d8b 100644 --- a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java @@ -1432,7 +1432,7 @@ public void copyFrom(int fromIndex, int thisIndex, ValueVector from) { BitVectorHelper.unsetBit(validityBuffer, thisIndex); } else { final int viewLength = from.getDataBuffer().getInt((long) fromIndex * ELEMENT_SIZE); - copyFromNotNull(fromIndex, thisIndex, from, viewLength); + copyFromNotNull(from, fromIndex, thisIndex, viewLength); } lastSet = thisIndex; } @@ -1454,39 +1454,35 @@ public void copyFromSafe(int fromIndex, int thisIndex, ValueVector from) { } else { final int viewLength = from.getDataBuffer().getInt((long) fromIndex * ELEMENT_SIZE); handleSafe(thisIndex, viewLength); - copyFromNotNull(fromIndex, thisIndex, from, viewLength); + copyFromNotNull(from, fromIndex, thisIndex, viewLength); } lastSet = thisIndex; } - private void copyFromNotNull(int fromIndex, int thisIndex, ValueVector from, int viewLength) { + private void copyFromNotNull(ValueVector from, int fromIndex, int thisIndex, int viewLength) { BitVectorHelper.setBit(validityBuffer, thisIndex); final int start = thisIndex * ELEMENT_SIZE; final int copyStart = fromIndex * ELEMENT_SIZE; if (viewLength > INLINE_SIZE) { - final int bufIndex = - from.getDataBuffer() - .getInt(((long) fromIndex * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH); - final int dataOffset = - from.getDataBuffer() - .getInt( - ((long) fromIndex * ELEMENT_SIZE) - + LENGTH_WIDTH - + PREFIX_WIDTH - + BUF_INDEX_WIDTH); - final ArrowBuf dataBuf = ((BaseVariableWidthViewVector) from).dataBuffers.get(bufIndex); - final ArrowBuf thisDataBuf = allocateOrGetLastDataBuffer(viewLength); - - viewBuffer.setBytes(start, from.getDataBuffer(), copyStart, LENGTH_WIDTH + PREFIX_WIDTH); - int writePosition = start + LENGTH_WIDTH + PREFIX_WIDTH; - // set buf id - viewBuffer.setInt(writePosition, dataBuffers.size() - 1); - writePosition += BUF_INDEX_WIDTH; - // set offset - viewBuffer.setInt(writePosition, (int) thisDataBuf.writerIndex()); - - thisDataBuf.setBytes(thisDataBuf.writerIndex(), dataBuf, dataOffset, viewLength); - thisDataBuf.writerIndex(thisDataBuf.writerIndex() + viewLength); + BaseVariableWidthViewVector fromVector = (BaseVariableWidthViewVector) from; + fromVector.getData( + fromIndex, + (dataBuf, dataOffset, dataLength) -> { + assert dataLength == viewLength; + viewBuffer.setBytes( + start, fromVector.getDataBuffer(), copyStart, LENGTH_WIDTH + PREFIX_WIDTH); + //noinspection resource + final ArrowBuf thisDataBuf = allocateOrGetLastDataBuffer(viewLength); + int writePosition = start + LENGTH_WIDTH + PREFIX_WIDTH; + // set buf id + viewBuffer.setInt(writePosition, dataBuffers.size() - 1); + writePosition += BUF_INDEX_WIDTH; + // set offset + viewBuffer.setInt(writePosition, (int) thisDataBuf.writerIndex()); + thisDataBuf.setBytes(thisDataBuf.writerIndex(), dataBuf, dataOffset, viewLength); + thisDataBuf.writerIndex(thisDataBuf.writerIndex() + viewLength); + return null; + }); } else { from.getDataBuffer().getBytes(copyStart, viewBuffer, start, ELEMENT_SIZE); } @@ -1502,16 +1498,12 @@ public ArrowBufPointer getDataPointer(int index, ArrowBufPointer reuse) { if (isNull(index)) { reuse.set(null, 0, 0); } else { - int length = getValueLength(index); - if (length < INLINE_SIZE) { - int start = index * ELEMENT_SIZE + LENGTH_WIDTH; - reuse.set(viewBuffer, start, length); - } else { - final int bufIndex = - viewBuffer.getInt(((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH); - ArrowBuf dataBuf = dataBuffers.get(bufIndex); - reuse.set(dataBuf, 0, length); - } + getData( + index, + (buf, offset, length) -> { + reuse.set(buf, offset, length); + return null; + }); } return reuse; } @@ -1526,19 +1518,43 @@ public int hashCode(int index, ArrowBufHasher hasher) { if (isNull(index)) { return ArrowBufPointer.NULL_HASH_CODE; } - final int length = getValueLength(index); - if (length < INLINE_SIZE) { - int start = index * ELEMENT_SIZE + LENGTH_WIDTH; - return ByteFunctionHelpers.hash(hasher, this.getDataBuffer(), start, start + length); - } else { - final int bufIndex = + return getData( + index, + (buf, offset, length) -> ByteFunctionHelpers.hash(hasher, buf, offset, offset + length)); + } + + @FunctionalInterface + protected interface ViewElementConsumer { + T consume(ArrowBuf buf, int offset, int length); + } + + /** Helper to get a single view value with sanity checking. */ + protected T getData(int index, ViewElementConsumer consumer) { + final int dataLength = getValueLength(index); + final ArrowBuf dataBuffer; + final int dataOffset; + if (dataLength > INLINE_SIZE) { + final int bufferIndex = viewBuffer.getInt(((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH); - final int dataOffset = + dataOffset = viewBuffer.getInt( ((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); - ArrowBuf dataBuf = dataBuffers.get(bufIndex); - return ByteFunctionHelpers.hash(hasher, dataBuf, dataOffset, dataOffset + length); + dataBuffer = dataBuffers.get(bufferIndex); + } else { + dataBuffer = viewBuffer; + dataOffset = index * ELEMENT_SIZE + BUF_INDEX_WIDTH; } + if (((long) dataOffset + (long) dataLength) > dataBuffer.capacity()) { + // In this case we don't check BOUNDS_CHECKING_ENABLED + // Likely this check is redundant, but we are trying to check eagerly before downstream code + // potentially + // tries to allocate based on the given dataLength + throw new IndexOutOfBoundsException( + String.format( + "index: %d, length: %d (expected: range(0, %d))", + dataOffset, dataLength, dataBuffer.capacity())); + } + return consumer.consume(dataBuffer, dataOffset, dataLength); } /** @@ -1555,40 +1571,16 @@ public int hashCode(int index, ArrowBufHasher hasher) { * @return byte array containing the data of the element */ protected byte[] getData(int index) { - final int dataLength = getValueLength(index); - if (dataLength > INLINE_SIZE) { - // data is in the data buffer - // get buffer index - final int bufferIndex = - viewBuffer.getInt(((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH); - // get data offset - final int dataOffset = - viewBuffer.getInt( - ((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); - ArrowBuf dataBuffer = dataBuffers.get(bufferIndex); - return dataBuffer.getBytesAsArray(dataOffset, dataLength); - } - // data is in the view buffer - return viewBuffer.getBytesAsArray((long) index * ELEMENT_SIZE + BUF_INDEX_WIDTH, dataLength); + return getData(index, ArrowBuf::getBytesAsArray); } protected void getData(int index, ReusableBuffer buffer) { - final int dataLength = getValueLength(index); - if (dataLength > INLINE_SIZE) { - // data is in the data buffer - // get buffer index - final int bufferIndex = - viewBuffer.getInt(((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH); - // get data offset - final int dataOffset = - viewBuffer.getInt( - ((long) index * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); - ArrowBuf dataBuf = dataBuffers.get(bufferIndex); - buffer.set(dataBuf, dataOffset, dataLength); - } else { - // data is in the value buffer - buffer.set(viewBuffer, ((long) index * ELEMENT_SIZE) + BUF_INDEX_WIDTH, dataLength); - } + getData( + index, + (buf, offset, length) -> { + buffer.set(buf, offset, length); + return null; + }); } @Override diff --git a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java index cdd687f4e5..b66d59e04d 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java @@ -2925,16 +2925,17 @@ public void testValidate() { @Test public void testValidateInvalidOffsets() { try (final ViewVarCharVector vector = new ViewVarCharVector("v", allocator)) { - vector.allocateNew(8, 1); + vector.allocateNew(16, 1); vector.allocateOrGetLastDataBuffer(8); var offsets = vector.getDataBuffer(); offsets.setInt(0, 64); - offsets.setInt(1, 0); - offsets.setInt(2, 0); - offsets.setInt(3, 1024); + offsets.setInt(4, 0); + offsets.setInt(8, 0); + offsets.setInt(12, 1024); vector.setValueCount(1); vector.setIndexDefined(0); - assertThrows(IndexOutOfBoundsException.class, vector::validateFull); + var e = assertThrows(IndexOutOfBoundsException.class, vector::validateFull); + assertTrue(e.getMessage().contains("index: 1024")); } } } From ed70a51c183cb13c9dd87e84c9c787301cb460fb Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 11 Sep 2026 14:07:51 +0900 Subject: [PATCH 4/6] another check --- .../apache/arrow/vector/BaseVariableWidthViewVector.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java index 8760f34d8b..4e838d0708 100644 --- a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java @@ -912,6 +912,12 @@ private void splitAndTransferViewBufferAndDataBuffer( viewBuffer.getInt( ((long) i * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); final ArrowBuf dataBuf = dataBuffers.get(readBufIndex); + if (((long) readBufOffset + (long) stringLength) > dataBuf.capacity()) { + throw new IndexOutOfBoundsException( + String.format( + "index: %d, length: %d (expected: range(0, %d))", + readBufOffset, stringLength, dataBuf.capacity())); + } // allocate data buffer ArrowBuf currentDataBuf = target.allocateOrGetLastDataBuffer(stringLength); From 7c635f483a2af386d16001c68aff425fcf1f3f3e Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 11 Sep 2026 16:14:01 +0900 Subject: [PATCH 5/6] more checking... --- .../apache/arrow/vector/BaseVariableWidthViewVector.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java index 4e838d0708..d9c48e6a85 100644 --- a/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java +++ b/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthViewVector.java @@ -912,7 +912,8 @@ private void splitAndTransferViewBufferAndDataBuffer( viewBuffer.getInt( ((long) i * ELEMENT_SIZE) + LENGTH_WIDTH + PREFIX_WIDTH + BUF_INDEX_WIDTH); final ArrowBuf dataBuf = dataBuffers.get(readBufIndex); - if (((long) readBufOffset + (long) stringLength) > dataBuf.capacity()) { + if (readBufOffset < 0 + || ((long) readBufOffset + (long) stringLength) > dataBuf.capacity()) { throw new IndexOutOfBoundsException( String.format( "index: %d, length: %d (expected: range(0, %d))", @@ -1550,7 +1551,9 @@ protected T getData(int index, ViewElementConsumer consumer) { dataBuffer = viewBuffer; dataOffset = index * ELEMENT_SIZE + BUF_INDEX_WIDTH; } - if (((long) dataOffset + (long) dataLength) > dataBuffer.capacity()) { + if (dataOffset < 0 + || dataLength < 0 + || ((long) dataOffset + (long) dataLength) > dataBuffer.capacity()) { // In this case we don't check BOUNDS_CHECKING_ENABLED // Likely this check is redundant, but we are trying to check eagerly before downstream code // potentially From 474c91cfc1aa7496b591d35fa2e014df429dfda2 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 11 Sep 2026 17:56:15 +0900 Subject: [PATCH 6/6] Change offset value to Integer.MAX_VALUE Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../org/apache/arrow/vector/TestVariableWidthViewVector.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java index b66d59e04d..2d33f57e99 100644 --- a/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java +++ b/vector/src/test/java/org/apache/arrow/vector/TestVariableWidthViewVector.java @@ -2928,7 +2928,7 @@ public void testValidateInvalidOffsets() { vector.allocateNew(16, 1); vector.allocateOrGetLastDataBuffer(8); var offsets = vector.getDataBuffer(); - offsets.setInt(0, 64); + offsets.setInt(0, Integer.MAX_VALUE); offsets.setInt(4, 0); offsets.setInt(8, 0); offsets.setInt(12, 1024);