Skip to content

Commit de73e14

Browse files
authored
MINOR: Bounds check delta dictionary data copy in VectorAppender (#1364)
## What's Changed `VectorAppender.visit(BaseVariableWidthVector)` and `visit(BaseLargeVariableWidthVector)` copy the delta's value bytes with `MemoryUtil.copyMemory`, starting at the delta's first offset for `lastOffset - firstOffset` bytes, and nothing checks that range against the delta data buffer. `ArrowReader.loadDictionary` passes IPC delta dictionary batches straight to `VectorBatchAppender.batchAppend`, so a stream whose VarChar delta dictionary has offsets `[0, 4, 4096]` over an 8-byte data buffer appends a 4092-byte dictionary value filled with adjacent native memory, with bounds checking enabled. Both visits now validate the delta before touching the target: the validity and offset buffers must cover the value count, and the offsets must start at a non-negative value, never decrease, and end within the data buffer. The target data size is computed with `Math.addExact`. These checks don't depend on `BoundsChecking`, and a rejected delta leaves the target unchanged. The data copy also uses `ArrowBuf.setBytes(index, src, srcIndex, length)`. Tests in `TestArrowReaderWriter`, for both VarChar and LargeVarChar: malformed delta offsets (past the data buffer, decreasing, last before first, negative start) throw `IndexOutOfBoundsException` and leave the original dictionary intact, and a valid delta with a non-zero start offset appends correctly.
1 parent 613fe80 commit de73e14

2 files changed

Lines changed: 207 additions & 10 deletions

File tree

‎vector/src/main/java/org/apache/arrow/vector/util/VectorAppender.java‎

Lines changed: 76 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,8 @@ public ValueVector visit(BaseVariableWidthVector deltaVector, Void value) {
119119
return targetVector; // nothing to append, return
120120
}
121121

122+
validateDelta(deltaVector);
123+
122124
int newValueCount = targetVector.getValueCount() + deltaVector.getValueCount();
123125

124126
int targetDataSize =
@@ -134,7 +136,7 @@ public ValueVector visit(BaseVariableWidthVector deltaVector, Void value) {
134136
.getOffsetBuffer()
135137
.getInt((long) deltaVector.getValueCount() * BaseVariableWidthVector.OFFSET_WIDTH)
136138
- deltaDataStart;
137-
int newValueCapacity = targetDataSize + deltaDataSize;
139+
int newValueCapacity = Math.addExact(targetDataSize, deltaDataSize);
138140

139141
// make sure there is enough capacity
140142
while (targetVector.getValueCapacity() < newValueCount) {
@@ -153,10 +155,9 @@ public ValueVector visit(BaseVariableWidthVector deltaVector, Void value) {
153155
targetVector.getValidityBuffer());
154156

155157
// append data buffer
156-
MemoryUtil.copyMemory(
157-
deltaVector.getDataBuffer().memoryAddress() + deltaDataStart,
158-
targetVector.getDataBuffer().memoryAddress() + targetDataSize,
159-
deltaDataSize);
158+
targetVector
159+
.getDataBuffer()
160+
.setBytes(targetDataSize, deltaVector.getDataBuffer(), deltaDataStart, deltaDataSize);
160161

161162
// copy offset buffer
162163
MemoryUtil.copyMemory(
@@ -194,6 +195,8 @@ public ValueVector visit(BaseLargeVariableWidthVector deltaVector, Void value) {
194195
return targetVector; // nothing to append, return
195196
}
196197

198+
validateDelta(deltaVector);
199+
197200
int newValueCount = targetVector.getValueCount() + deltaVector.getValueCount();
198201

199202
long targetDataSize =
@@ -210,7 +213,7 @@ public ValueVector visit(BaseLargeVariableWidthVector deltaVector, Void value) {
210213
.getLong(
211214
(long) deltaVector.getValueCount() * BaseLargeVariableWidthVector.OFFSET_WIDTH)
212215
- deltaDataStart;
213-
long newValueCapacity = targetDataSize + deltaDataSize;
216+
long newValueCapacity = Math.addExact(targetDataSize, deltaDataSize);
214217

215218
// make sure there is enough capacity
216219
while (targetVector.getValueCapacity() < newValueCount) {
@@ -229,10 +232,9 @@ public ValueVector visit(BaseLargeVariableWidthVector deltaVector, Void value) {
229232
targetVector.getValidityBuffer());
230233

231234
// append data buffer
232-
MemoryUtil.copyMemory(
233-
deltaVector.getDataBuffer().memoryAddress() + deltaDataStart,
234-
targetVector.getDataBuffer().memoryAddress() + targetDataSize,
235-
deltaDataSize);
235+
targetVector
236+
.getDataBuffer()
237+
.setBytes(targetDataSize, deltaVector.getDataBuffer(), deltaDataStart, deltaDataSize);
236238

237239
// copy offset buffer
238240
MemoryUtil.copyMemory(
@@ -261,6 +263,70 @@ public ValueVector visit(BaseLargeVariableWidthVector deltaVector, Void value) {
261263
return targetVector;
262264
}
263265

266+
/**
267+
* Checks that the delta's validity and offset buffers cover its value count, and that its offsets
268+
* are non-negative, non-decreasing and within its data buffer. This runs before the target is
269+
* touched, so a malformed delta leaves the target unchanged.
270+
*/
271+
private static void validateDelta(BaseVariableWidthVector deltaVector) {
272+
int valueCount = deltaVector.getValueCount();
273+
checkCapacity(
274+
deltaVector.getValidityBuffer(),
275+
BitVectorHelper.getValidityBufferSizeFromCount(valueCount),
276+
"validity");
277+
ArrowBuf offsets = deltaVector.getOffsetBuffer();
278+
checkCapacity(offsets, (valueCount + 1L) * BaseVariableWidthVector.OFFSET_WIDTH, "offset");
279+
int prev = offsets.getInt(0);
280+
if (prev < 0) {
281+
throw new IndexOutOfBoundsException("Delta vector has negative start offset " + prev);
282+
}
283+
for (int i = 1; i <= valueCount; i++) {
284+
int cur = offsets.getInt((long) i * BaseVariableWidthVector.OFFSET_WIDTH);
285+
if (cur < prev) {
286+
throw new IndexOutOfBoundsException(
287+
"Delta vector offsets decrease at index " + i + ": " + prev + " > " + cur);
288+
}
289+
prev = cur;
290+
}
291+
checkCapacity(deltaVector.getDataBuffer(), prev, "data");
292+
}
293+
294+
/** See {@link #validateDelta(BaseVariableWidthVector)}. */
295+
private static void validateDelta(BaseLargeVariableWidthVector deltaVector) {
296+
int valueCount = deltaVector.getValueCount();
297+
checkCapacity(
298+
deltaVector.getValidityBuffer(),
299+
BitVectorHelper.getValidityBufferSizeFromCount(valueCount),
300+
"validity");
301+
ArrowBuf offsets = deltaVector.getOffsetBuffer();
302+
checkCapacity(offsets, (valueCount + 1L) * BaseLargeVariableWidthVector.OFFSET_WIDTH, "offset");
303+
long prev = offsets.getLong(0);
304+
if (prev < 0) {
305+
throw new IndexOutOfBoundsException("Delta vector has negative start offset " + prev);
306+
}
307+
for (int i = 1; i <= valueCount; i++) {
308+
long cur = offsets.getLong((long) i * BaseLargeVariableWidthVector.OFFSET_WIDTH);
309+
if (cur < prev) {
310+
throw new IndexOutOfBoundsException(
311+
"Delta vector offsets decrease at index " + i + ": " + prev + " > " + cur);
312+
}
313+
prev = cur;
314+
}
315+
checkCapacity(deltaVector.getDataBuffer(), prev, "data");
316+
}
317+
318+
private static void checkCapacity(ArrowBuf buffer, long required, String name) {
319+
if (buffer.capacity() < required) {
320+
throw new IndexOutOfBoundsException(
321+
"Delta vector "
322+
+ name
323+
+ " buffer has capacity "
324+
+ buffer.capacity()
325+
+ ", needs "
326+
+ required);
327+
}
328+
}
329+
264330
@Override
265331
public ValueVector visit(BaseVariableWidthViewVector deltaVector, Void value) {
266332
Preconditions.checkArgument(

‎vector/src/test/java/org/apache/arrow/vector/ipc/TestArrowReaderWriter.java‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
import java.util.Map;
4646
import java.util.function.BiFunction;
4747
import java.util.stream.Collectors;
48+
import java.util.stream.Stream;
4849
import org.apache.arrow.flatbuf.FieldNode;
4950
import org.apache.arrow.flatbuf.Message;
5051
import org.apache.arrow.flatbuf.RecordBatch;
@@ -53,12 +54,15 @@
5354
import org.apache.arrow.memory.RootAllocator;
5455
import org.apache.arrow.util.AutoCloseables;
5556
import org.apache.arrow.util.Collections2;
57+
import org.apache.arrow.vector.BitVectorHelper;
5658
import org.apache.arrow.vector.FieldVector;
5759
import org.apache.arrow.vector.IntVector;
60+
import org.apache.arrow.vector.LargeVarCharVector;
5861
import org.apache.arrow.vector.NullVector;
5962
import org.apache.arrow.vector.TestUtils;
6063
import org.apache.arrow.vector.ValueVector;
6164
import org.apache.arrow.vector.VarCharVector;
65+
import org.apache.arrow.vector.VariableWidthFieldVector;
6266
import org.apache.arrow.vector.VectorLoader;
6367
import org.apache.arrow.vector.VectorSchemaRoot;
6468
import org.apache.arrow.vector.VectorUnloader;
@@ -89,6 +93,10 @@
8993
import org.junit.jupiter.api.AfterEach;
9094
import org.junit.jupiter.api.BeforeEach;
9195
import org.junit.jupiter.api.Test;
96+
import org.junit.jupiter.params.ParameterizedTest;
97+
import org.junit.jupiter.params.provider.Arguments;
98+
import org.junit.jupiter.params.provider.MethodSource;
99+
import org.junit.jupiter.params.provider.ValueSource;
92100

93101
public class TestArrowReaderWriter {
94102

@@ -649,6 +657,129 @@ public void testDeltaDictionary() throws Exception {
649657
AutoCloseables.close(closeableList);
650658
}
651659

660+
@ParameterizedTest
661+
@ValueSource(booleans = {false, true})
662+
public void testDeltaDictionaryWithNonZeroStartOffset(boolean large) throws Exception {
663+
// a valid delta whose offsets start at 3, so its data begins after a 3-byte prefix
664+
byte[] stream = writeDeltaDictionaryStream(large, new long[] {3, 6, 9}, "xxxbazqux");
665+
try (ArrowStreamReader reader =
666+
new ArrowStreamReader(new ByteArrayReadableSeekableByteChannel(stream), allocator)) {
667+
assertFalse(reader.loadNextBatch());
668+
FieldVector dictionaryVector = reader.getDictionaryVectors().get(1L).getVector();
669+
assertEquals(4, dictionaryVector.getValueCount());
670+
assertEquals("foo", dictionaryVector.getObject(0).toString());
671+
assertEquals("bar", dictionaryVector.getObject(1).toString());
672+
assertEquals("baz", dictionaryVector.getObject(2).toString());
673+
assertEquals("qux", dictionaryVector.getObject(3).toString());
674+
}
675+
}
676+
677+
@ParameterizedTest
678+
@MethodSource("malformedDeltaOffsets")
679+
public void testDeltaDictionaryWithMalformedOffsets(boolean large, long[] deltaOffsets)
680+
throws Exception {
681+
byte[] stream = writeDeltaDictionaryStream(large, deltaOffsets, "bazzquxx");
682+
try (ArrowStreamReader reader =
683+
new ArrowStreamReader(new ByteArrayReadableSeekableByteChannel(stream), allocator)) {
684+
assertThrows(IndexOutOfBoundsException.class, reader::loadNextBatch);
685+
// the rejected delta must leave the original dictionary untouched
686+
FieldVector dictionaryVector = reader.getDictionaryVectors().get(1L).getVector();
687+
assertEquals(2, dictionaryVector.getValueCount());
688+
assertEquals("foo", dictionaryVector.getObject(0).toString());
689+
assertEquals("bar", dictionaryVector.getObject(1).toString());
690+
assertEquals(0, BitVectorHelper.get(dictionaryVector.getValidityBuffer(), 2));
691+
assertEquals(0, BitVectorHelper.get(dictionaryVector.getValidityBuffer(), 3));
692+
}
693+
}
694+
695+
static Stream<Arguments> malformedDeltaOffsets() {
696+
List<long[]> offsets =
697+
Arrays.asList(
698+
new long[] {0, 4, 4096}, // last offset past the 8-byte data buffer
699+
new long[] {0, 6, 4}, // decreasing intermediate offset
700+
new long[] {4, 0, 0}, // last offset before the first
701+
new long[] {-4, 0, 4}); // negative start offset
702+
return Stream.of(false, true)
703+
.flatMap(large -> offsets.stream().map(o -> Arguments.of(large, o)));
704+
}
705+
706+
/**
707+
* Writes a stream holding a {"foo", "bar"} dictionary followed by a delta dictionary batch built
708+
* directly from the given offsets and data bytes, bypassing any vector-level validation.
709+
*/
710+
private byte[] writeDeltaDictionaryStream(boolean large, long[] deltaOffsets, String deltaData)
711+
throws IOException {
712+
List<AutoCloseable> closeables = new ArrayList<>();
713+
try {
714+
FieldVector dictVector =
715+
large
716+
? newVector(LargeVarCharVector.class, "dict", MinorType.LARGEVARCHAR, allocator)
717+
: newVarCharVector("dict", allocator);
718+
closeables.add(dictVector);
719+
dictVector.allocateNew();
720+
((VariableWidthFieldVector) dictVector).setSafe(0, "foo".getBytes(StandardCharsets.UTF_8));
721+
((VariableWidthFieldVector) dictVector).setSafe(1, "bar".getBytes(StandardCharsets.UTF_8));
722+
dictVector.setValueCount(2);
723+
Dictionary dictionary = new Dictionary(dictVector, new DictionaryEncoding(1L, false, null));
724+
DictionaryProvider.MapDictionaryProvider provider =
725+
new DictionaryProvider.MapDictionaryProvider();
726+
provider.put(dictionary);
727+
Field encodedField =
728+
new Field(
729+
"f",
730+
new FieldType(true, new ArrowType.Int(32, true), dictionary.getEncoding()),
731+
null);
732+
Schema schema =
733+
new Schema(
734+
Collections.singletonList(
735+
DictionaryUtility.toMessageFormat(encodedField, provider, new HashSet<>())));
736+
737+
ByteArrayOutputStream outStream = new ByteArrayOutputStream();
738+
WriteChannel out = new WriteChannel(newChannel(outStream));
739+
MessageSerializer.serialize(out, schema);
740+
serializeDictionaryBatch(out, dictionary, false, closeables);
741+
742+
int offsetWidth = large ? 8 : 4;
743+
ArrowBuf validity = allocator.buffer(0);
744+
closeables.add(validity);
745+
ArrowBuf offsets = allocator.buffer((long) deltaOffsets.length * offsetWidth);
746+
closeables.add(offsets);
747+
for (int i = 0; i < deltaOffsets.length; i++) {
748+
if (large) {
749+
offsets.setLong((long) i * offsetWidth, deltaOffsets[i]);
750+
} else {
751+
offsets.setInt((long) i * offsetWidth, (int) deltaOffsets[i]);
752+
}
753+
}
754+
offsets.writerIndex((long) deltaOffsets.length * offsetWidth);
755+
byte[] dataBytes = deltaData.getBytes(StandardCharsets.UTF_8);
756+
ArrowBuf data = allocator.buffer(dataBytes.length);
757+
closeables.add(data);
758+
data.setBytes(0, dataBytes);
759+
data.writerIndex(dataBytes.length);
760+
761+
int deltaValueCount = deltaOffsets.length - 1;
762+
ArrowRecordBatch deltaBatch =
763+
new ArrowRecordBatch(
764+
deltaValueCount,
765+
Collections.singletonList(new ArrowFieldNode(deltaValueCount, 0)),
766+
Arrays.asList(validity, offsets, data));
767+
ArrowDictionaryBatch deltaDictionaryBatch = new ArrowDictionaryBatch(1L, deltaBatch, true);
768+
closeables.add(deltaDictionaryBatch);
769+
MessageSerializer.serialize(out, deltaDictionaryBatch);
770+
771+
// write eos
772+
out.writeIntLittleEndian(0);
773+
return outStream.toByteArray();
774+
} finally {
775+
try {
776+
AutoCloseables.close(closeables);
777+
} catch (Exception e) {
778+
throw new IOException(e);
779+
}
780+
}
781+
}
782+
652783
// Tests that the ArrowStreamWriter re-emits dictionaries when they change
653784
@Test
654785
public void testWriteReadStreamWithDictionaryReplacement() throws Exception {

0 commit comments

Comments
 (0)