Skip to content

GH-1261: Reject out-of-range dictionary indices in decode #1262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Arawoof06 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GH-1261: Reject out-of-range dictionary indices in decode #1262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vector/src/main/java/org/apache/arrow/vector/dictionary/DictionaryEncoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ static void retrieveIndexVector(
for (int i = start; i < end; i++) {
if (!indices.isNull(i)) {
int indexAsInt = (int) indices.getValueAsLong(i);
if (indexAsInt > dictionaryCount) {
if (indexAsInt < 0 || indexAsInt >= dictionaryCount) {

@jbonofre jbonofre Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper has a caller that doesn't pass a dictionary size. StructSubfieldEncoder.decode calls:

DictionaryEncoder.retrieveIndexVector(indices, transfer, valueCount, 0, valueCount);

where valueCount is the struct's row count. With >= that gives two problems:

  • A 1-row struct whose f0 child holds index 1 into a 2-entry dictionary ["aa", "bb"] decodes on main and now throws Provided dictionary does not contain value for index 1.
  • A 40-row struct with index 39 into a 1-entry dictionary passes 39 >= 40 and reaches copyValueSafe. For a fixed-width dictionary that ends in a raw MemoryUtil.copyMemory in BaseFixedWidthVector.copyFrom.

Could you pass dictionary.getVector().getValueCount() in StructSubfieldEncoder instead? The PR description says the struct decoder is covered by this change, which only holds once that bound is fixed.

throw new IllegalArgumentException(
"Provided dictionary does not contain value for index " + indexAsInt);
}
Comment on lines 167 to 171

@jbonofre jbonofre Oct 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The narrowing to int happens before the range check, so a 64-bit index can wrap into range. With a BigIntVector index, 4294967296 becomes 0 and -4294967295 becomes 1, and both are accepted. A UInt4 index of 4294967295 is rejected, but the message reports index -1.

Checking the long first fixes both:

Suggested change
int indexAsInt = (int) indices.getValueAsLong(i);
if (indexAsInt > dictionaryCount) {
if (indexAsInt < 0 || indexAsInt >= dictionaryCount) {
throw new IllegalArgumentException(
"Provided dictionary does not contain value for index " + indexAsInt);
}
long index = indices.getValueAsLong(i);
if (index < 0 || index >= dictionaryCount) {
throw new IllegalArgumentException(
"Provided dictionary does not contain value for index " + index);
}
int indexAsInt = (int) index;

Expand Down
29 changes: 28 additions & 1 deletion vector/src/test/java/org/apache/arrow/vector/TestDictionaryVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,33 @@ public void testNoMemoryLeak() {
assertEquals(0, allocator.getAllocatedMemory(), "decode memory leak");
}

@Test
public void testDecodeIndexOutOfBounds() {
// valid indices are 0..dictionaryCount-1; index == dictionaryCount and negative indices
// must be rejected before dereferencing the dictionary vector.
try (final IntVector indices = newVector(IntVector.class, "", Types.MinorType.INT, allocator);
final VarCharVector dictionaryVector = newVarCharVector("dict", allocator)) {
setVector(dictionaryVector, zero, one);
Dictionary dictionary =
new Dictionary(dictionaryVector, new DictionaryEncoding(1L, false, null));

setVector(indices, 2);
try (final ValueVector decoded = DictionaryEncoder.decode(indices, dictionary, allocator)) {
fail("There should be an exception when decoding an index equal to the dictionary size");
} catch (IllegalArgumentException e) {
assertEquals("Provided dictionary does not contain value for index 2", e.getMessage());
}

setVector(indices, -1);
try (final ValueVector decoded = DictionaryEncoder.decode(indices, dictionary, allocator)) {
fail("There should be an exception when decoding a negative index");
} catch (IllegalArgumentException e) {
assertEquals("Provided dictionary does not contain value for index -1", e.getMessage());
}
}
assertEquals(0, allocator.getAllocatedMemory(), "decode memory leak");
}

@Test
public void testListNoMemoryLeak() {
// Create a new value vector
Expand Down Expand Up @@ -1053,7 +1080,7 @@ public void testStructNoMemoryLeak() {
NullableStructWriter writer = indices.getWriter();
writer.allocate();
writer.start();
writer.integer("f0").writeInt(1);
writer.integer("f0").writeInt(0);
writer.integer("f1").writeInt(3);
writer.end();
writer.setValueCount(1);
Expand Down
Loading