Skip to content

GH-1246: validate decimal byte length before writing in setBigEndian #1247

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
+65 −4
Open

GH-1246: validate decimal byte length before writing in setBigEndian #1247

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
15 changes: 13 additions & 2 deletions vector/src/main/java/org/apache/arrow/vector/Decimal256Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,14 @@ public void setBigEndian(int index, byte[] value) {
BitVectorHelper.setBit(validityBuffer, index);
final int length = value.length;

// Reject an oversized value before any bytes are written. The little-endian path below
// copies all `length` bytes into the fixed TYPE_WIDTH slot with unchecked native writes,
// so validating after the copy would leave adjacent memory already corrupted.
if (length > TYPE_WIDTH) {

@jbonofre jbonofre Oct 7, 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.

Same issue here as in DecimalVector.setBigEndian. setBit runs before this check, so a value longer than 32 bytes leaves the slot marked valid after the exception.

Please validate the length first.

throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 32], got " + length);
}

// do the bound check.
valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH);

Expand Down Expand Up @@ -243,8 +251,6 @@ public void setBigEndian(int index, byte[] value) {
return;
}
}
throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 32], got " + length);
}

/**
Expand Down Expand Up @@ -308,6 +314,11 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length)
handleSafe(index);
BitVectorHelper.setBit(validityBuffer, index);

if (length > TYPE_WIDTH) {

@jbonofre jbonofre Oct 7, 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.

Same here as in DecimalVector.setBigEndianSafe.

Please validate length before handleSafe and setBit.

throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 32], got " + length);
}

// do the bound checks.
buffer.checkBytes(start, start + length);
valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH);
Expand Down
15 changes: 13 additions & 2 deletions vector/src/main/java/org/apache/arrow/vector/DecimalVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,14 @@ public void setBigEndian(int index, byte[] value) {
BitVectorHelper.setBit(validityBuffer, index);
final int length = value.length;

// Reject an oversized value before any bytes are written. The little-endian path below
// copies all `length` bytes into the fixed TYPE_WIDTH slot with unchecked native writes,
// so validating after the copy would leave adjacent memory already corrupted.
if (length > TYPE_WIDTH) {

@jbonofre jbonofre Oct 7, 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.

BitVectorHelper.setBit(validityBuffer, index) a few lines above still runs before this check. If an oversized value is rejected here, the slot is already marked valid. A caller that catches the exception then sees isNull(index) == false and reads stale or zero bytes instead of null.

Could you move this check above the setBit call, so a rejected call leaves the vector unchanged?

throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 16], got " + length);
}

// do the bound check.
valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH);

Expand Down Expand Up @@ -241,8 +249,6 @@ public void setBigEndian(int index, byte[] value) {
return;
}
}
throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 16], got " + length);
}

/**
Expand Down Expand Up @@ -306,6 +312,11 @@ public void setBigEndianSafe(int index, long start, ArrowBuf buffer, int length)
handleSafe(index);
BitVectorHelper.setBit(validityBuffer, index);

if (length > TYPE_WIDTH) {

@jbonofre jbonofre Oct 7, 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.

handleSoft(index) and setBit both run before this check. A rejected oversize call can still grow the vector and mark the slot valid.

Please validate length at the top of the method so a failed call has no side effects.

throw new IllegalArgumentException(
"Invalid decimal value length. Valid length in [1 - 16], got " + length);
}

// do the bound checks.
buffer.checkBytes(start, start + length);
valueBuffer.checkBytes((long) index * TYPE_WIDTH, (long) (index + 1) * TYPE_WIDTH);
Expand Down
19 changes: 19 additions & 0 deletions vector/src/test/java/org/apache/arrow/vector/TestDecimal256Vector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,25 @@ public void decimalBE2NE() {
}
}

@Test
public void setBigEndianOversizedDoesNotCorruptNeighbor() {
try (Decimal256Vector decimalVector =
TestUtils.newVector(
Decimal256Vector.class, "decimal", new ArrowType.Decimal(60, 0, 256), allocator)) {
decimalVector.allocateNew(2);

// A value whose low bytes are all non-zero, stored in the slot right after the target.
final BigInteger neighbor = new BigInteger("305419896"); // 0x12345678
decimalVector.setBigEndian(1, neighbor.toByteArray());

// A value longer than the 32-byte decimal must be rejected before anything is written.
assertThrows(
IllegalArgumentException.class, () -> decimalVector.setBigEndian(0, new byte[40]));

assertEquals(neighbor, decimalVector.getObject(1).unscaledValue());

@jbonofre jbonofre Oct 7, 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.

Same as the TestDecimalVector comment. Please assert isNull(0) after the rejected call, and add a setBigEndianSafe case.

}
}

@Test
public void setUsingArrowBufOfLEInts() {
try (Decimal256Vector decimalVector =
Expand Down
20 changes: 20 additions & 0 deletions vector/src/test/java/org/apache/arrow/vector/TestDecimalVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
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;

Expand Down Expand Up @@ -279,6 +280,25 @@ public void decimalBE2NE() {
}
}

@Test
public void setBigEndianOversizedDoesNotCorruptNeighbor() {
try (DecimalVector decimalVector =
TestUtils.newVector(
DecimalVector.class, "decimal", new ArrowType.Decimal(38, 0, 128), allocator)) {
decimalVector.allocateNew(2);

// A value whose low bytes are all non-zero, stored in the slot right after the target.
final BigInteger neighbor = new BigInteger("305419896"); // 0x12345678
decimalVector.setBigEndian(1, neighbor.toByteArray());

// A value longer than the 16-byte decimal must be rejected before anything is written.
assertThrows(
IllegalArgumentException.class, () -> decimalVector.setBigEndian(0, new byte[24]));

assertEquals(neighbor, decimalVector.getObject(1).unscaledValue());

@jbonofre jbonofre Oct 7, 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 covers the original bug, since the neighboring slot stays intact. It doesn't check the target slot after the exception.

Once the check is moved, please also assert assertTrue(decimalVector.isNull(0)) after the failed call. That would catch the validity-bit ordering issue.

A matching test for setBigEndianSafe would help too. It should assert isNull and that the value capacity is unchanged after the rejected call.

}
}

@Test
public void setUsingArrowBufOfInts() {
try (DecimalVector decimalVector =
Expand Down
Loading