Skip to content

Commit 3e47560

Browse files
authored
GH-1098: Fix always-true precondition check in LargeListVector.setValueCount (#1099)
## What's Changed Fix the precondition check in `LargeListVector.setValueCount()` where `||` made the condition always true (`childValueCount <= Integer.MAX_VALUE || childValueCount >= Integer.MIN_VALUE` is a tautology for any `long` value), allowing silent data corruption when `childValueCount` exceeds `Integer.MAX_VALUE` or is negative. Changes: - Replace `||` with `&&` and `Integer.MIN_VALUE` with `0` to correctly bound `childValueCount` to `[0, Integer.MAX_VALUE]`, consistent with `LargeListViewVector` - Add rejection tests for negative and overflow `childValueCount` - Add positive boundary tests for zero, valid, and `Integer.MAX_VALUE` values Closes #1098.
1 parent 4b594b6 commit 3e47560

2 files changed

Lines changed: 91 additions & 2 deletions

File tree

‎vector/src/main/java/org/apache/arrow/vector/complex/LargeListVector.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1041,8 +1041,8 @@ public void setValueCount(int valueCount) {
10411041
* TODO: revisit when 64-bit vectors are supported
10421042
*/
10431043
Preconditions.checkArgument(
1044-
childValueCount <= Integer.MAX_VALUE || childValueCount >= Integer.MIN_VALUE,
1045-
"LargeListVector doesn't yet support 64-bit allocations: %s",
1044+
childValueCount >= 0 && childValueCount <= Integer.MAX_VALUE,
1045+
"LargeListVector childValueCount must be in [0, Integer.MAX_VALUE] but was: %s",
10461046
childValueCount);
10471047
vector.setValueCount((int) childValueCount);
10481048
}

‎vector/src/test/java/org/apache/arrow/vector/TestLargeListVector.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,17 @@
2121
import static org.junit.jupiter.api.Assertions.assertFalse;
2222
import static org.junit.jupiter.api.Assertions.assertNull;
2323
import static org.junit.jupiter.api.Assertions.assertSame;
24+
import static org.junit.jupiter.api.Assertions.assertThrows;
2425
import static org.junit.jupiter.api.Assertions.assertTrue;
26+
import static org.junit.jupiter.api.Assertions.fail;
2527

2628
import java.util.ArrayList;
2729
import java.util.Arrays;
2830
import java.util.List;
2931
import java.util.UUID;
3032
import org.apache.arrow.memory.ArrowBuf;
3133
import org.apache.arrow.memory.BufferAllocator;
34+
import org.apache.arrow.memory.OutOfMemoryException;
3235
import org.apache.arrow.vector.complex.BaseRepeatedValueVector;
3336
import org.apache.arrow.vector.complex.LargeListVector;
3437
import org.apache.arrow.vector.complex.ListVector;
@@ -42,6 +45,7 @@
4245
import org.apache.arrow.vector.types.pojo.ArrowType;
4346
import org.apache.arrow.vector.types.pojo.Field;
4447
import org.apache.arrow.vector.types.pojo.FieldType;
48+
import org.apache.arrow.vector.util.OversizedAllocationException;
4549
import org.apache.arrow.vector.util.TransferPair;
4650
import org.apache.arrow.vector.util.UuidUtility;
4751
import org.junit.jupiter.api.AfterEach;
@@ -1127,4 +1131,89 @@ private void writeIntValues(UnionLargeListWriter writer, int[] values) {
11271131
}
11281132
writer.endList();
11291133
}
1134+
1135+
@Test
1136+
public void testSetValueCountRejectsNegativeChildValueCount() {
1137+
try (final LargeListVector vector = LargeListVector.empty("list", allocator)) {
1138+
vector.addOrGetVector(FieldType.nullable(MinorType.INT.getType()));
1139+
vector.allocateNew();
1140+
1141+
// Write a negative value into the offset buffer to simulate a corrupted offset.
1142+
// The Preconditions check should reject any negative childValueCount.
1143+
vector.getOffsetBuffer().setLong(LargeListVector.OFFSET_WIDTH, -1L);
1144+
vector.setLastSet(0);
1145+
1146+
assertThrows(
1147+
IllegalArgumentException.class,
1148+
() -> vector.setValueCount(1),
1149+
"setValueCount should reject negative childValueCount");
1150+
}
1151+
}
1152+
1153+
@Test
1154+
public void testSetValueCountRejectsOverflowChildValueCount() {
1155+
try (final LargeListVector vector = LargeListVector.empty("list", allocator)) {
1156+
vector.addOrGetVector(FieldType.nullable(MinorType.INT.getType()));
1157+
vector.allocateNew();
1158+
1159+
// Write a value exceeding Integer.MAX_VALUE into the offset buffer
1160+
vector.getOffsetBuffer().setLong(LargeListVector.OFFSET_WIDTH, (long) Integer.MAX_VALUE + 1L);
1161+
vector.setLastSet(0);
1162+
1163+
assertThrows(
1164+
IllegalArgumentException.class,
1165+
() -> vector.setValueCount(1),
1166+
"setValueCount should reject childValueCount exceeding Integer.MAX_VALUE");
1167+
}
1168+
}
1169+
1170+
@Test
1171+
public void testSetValueCountAcceptsZeroChildValueCount() {
1172+
try (final LargeListVector vector = LargeListVector.empty("list", allocator)) {
1173+
vector.addOrGetVector(FieldType.nullable(MinorType.INT.getType()));
1174+
vector.allocateNew();
1175+
1176+
// childValueCount = 0 when valueCount = 0 (no elements)
1177+
vector.setValueCount(0);
1178+
assertEquals(0, vector.getValueCount());
1179+
}
1180+
}
1181+
1182+
@Test
1183+
public void testSetValueCountAcceptsValidChildValueCount() {
1184+
try (final LargeListVector vector = LargeListVector.empty("list", allocator)) {
1185+
vector.addOrGetVector(FieldType.nullable(MinorType.INT.getType()));
1186+
vector.allocateNew();
1187+
1188+
// Write a valid childValueCount (5) into the offset buffer
1189+
vector.getOffsetBuffer().setLong(LargeListVector.OFFSET_WIDTH, 5L);
1190+
vector.setLastSet(0);
1191+
vector.setValueCount(1);
1192+
assertEquals(1, vector.getValueCount());
1193+
}
1194+
}
1195+
1196+
@Test
1197+
public void testSetValueCountAcceptsMaxIntChildValueCount() {
1198+
try (final LargeListVector vector = LargeListVector.empty("list", allocator)) {
1199+
vector.addOrGetVector(FieldType.nullable(MinorType.INT.getType()));
1200+
vector.allocateNew();
1201+
1202+
// Write Integer.MAX_VALUE into the offset buffer to verify the upper boundary is inclusive.
1203+
// The Preconditions check should accept this value (not throw IllegalArgumentException).
1204+
// The child vector may throw OversizedAllocationException due to memory limits,
1205+
// which is expected and unrelated to the precondition validation.
1206+
vector.getOffsetBuffer().setLong(LargeListVector.OFFSET_WIDTH, Integer.MAX_VALUE);
1207+
vector.setLastSet(0);
1208+
try {
1209+
vector.setValueCount(1);
1210+
} catch (IllegalArgumentException e) {
1211+
fail("setValueCount should not reject childValueCount = Integer.MAX_VALUE", e);
1212+
} catch (OversizedAllocationException | OutOfMemoryException e) {
1213+
// OversizedAllocationException or other allocation errors are expected
1214+
// when trying to allocate Integer.MAX_VALUE elements — this is fine,
1215+
// the precondition check itself passed.
1216+
}
1217+
}
1218+
}
11301219
}

0 commit comments

Comments
 (0)