This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-lang.git
The following commit(s) were added to refs/heads/master by this push:
new 8dcf60003 BitField long-mask fields silently mis-answer int/short/byte
accessors: shifts masked mod 32, and sign extension makes above-bit-31 flags
read as set on any negative holder (f019).
8dcf60003 is described below
commit 8dcf60003f43df55003f901026ba15f6b6edea74
Author: Gary Gregory <[email protected]>
AuthorDate: Sat Sep 5 14:32:35 2026 -0400
BitField long-mask fields silently mis-answer int/short/byte accessors:
shifts masked mod 32, and sign extension makes above-bit-31 flags read
as set on any negative holder (f019).
---
src/changes/changes.xml | 1 +
.../java/org/apache/commons/lang3/BitField.java | 54 +++++++++++++++++++---
.../org/apache/commons/lang3/BitFieldLongTest.java | 32 +++++++------
.../org/apache/commons/lang3/BitFieldTest.java | 38 +++++++++++++++
4 files changed, 105 insertions(+), 20 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index f2bc1963f..82bc504eb 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -268,6 +268,7 @@ java.lang.NullPointerException: Cannot invoke
<action type="fix" dev="ggregory" due-to="Gary
Gregory">Match Javadoc to code in SystemProperties.getProperty() and
SystemUtils.getEnvironmentVariable() and update inline comments (f016).</action>
<action type="fix" dev="ggregory" due-to="Gary
Gregory">Fix LockingVisitors Javadoc to match code (f017).</action>
<action type="fix" dev="ggregory" due-to="Gary
Gregory">DoubleRange/Range accept NaN endpoints, silently producing a
half-unbounded range, contains()/fit() fail open for every value above the
minimum (f018).</action>
+ <action type="fix" dev="ggregory" due-to="Gary
Gregory">BitField long-mask fields silently mis-answer int/short/byte
accessors: shifts masked mod 32, and sign extension makes above-bit-31 flags
read as set on any negative holder (f019).</action>
<!-- ADD -->
<action type="add" dev="ggregory" due-to="Gary
Gregory">Add JavaVersion.JAVA_27.</action>
<action type="add" dev="ggregory" due-to="Gary
Gregory">Add SystemUtils.IS_JAVA_27.</action>
diff --git a/src/main/java/org/apache/commons/lang3/BitField.java
b/src/main/java/org/apache/commons/lang3/BitField.java
index 177a84d3f..28adb9ae9 100644
--- a/src/main/java/org/apache/commons/lang3/BitField.java
+++ b/src/main/java/org/apache/commons/lang3/BitField.java
@@ -88,6 +88,11 @@ public BitField(final int mask) {
/**
* Creates a BitField instance.
+ * <p>
+ * If any bit above bit 31 is set in the mask, the resulting field can
only be used with the {@code long} holder accessors; the {@code int}, {@code
short}
+ * and {@code byte} holder accessors throw {@link IllegalStateException}
for such a field, because those holder types cannot contain the masked bits and
+ * would otherwise silently answer wrongly (shift counts are truncated mod
32 and negative holders are sign-extended into bits 32-63).
+ * </p>
*
* @param mask The mask specifying which bits apply to this BitField. Bits
that are set in this mask are the bits that this BitField operates on.
* @since 3.21.0
@@ -102,9 +107,10 @@ public BitField(final long mask) {
*
* @param holder The int data containing the bits we're interested in.
* @return The value of holder with the specified bits cleared (set to
{@code 0}).
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int clear(final int holder) {
- return (int) (holder & ~mask);
+ return (int) (holder & ~intMask());
}
/**
@@ -123,6 +129,7 @@ public long clear(final long holder) {
*
* @param holder The byte data containing the bits we're interested in.
* @return The value of holder with the specified bits cleared (set to
{@code 0}).
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public byte clearByte(final byte holder) {
return (byte) clear(holder);
@@ -133,6 +140,7 @@ public byte clearByte(final byte holder) {
*
* @param holder The short data containing the bits we're interested in.
* @return The value of holder with the specified bits cleared (set to
{@code 0}).
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short clearShort(final short holder) {
return (short) clear(holder);
@@ -143,9 +151,10 @@ public short clearShort(final short holder) {
*
* @param holder The int data containing the bits we're interested in.
* @return The selected bits.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int getRawValue(final int holder) {
- return (int) (holder & mask);
+ return (int) (holder & intMask());
}
/**
@@ -164,6 +173,7 @@ public long getRawValue(final long holder) {
*
* @param holder The short data containing the bits we're interested in.
* @return The selected bits.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short getShortRawValue(final short holder) {
return (short) getRawValue(holder);
@@ -179,6 +189,7 @@ public short getShortRawValue(final short holder) {
* @param holder The short data containing the bits we're interested in.
* @return The selected bits, shifted right appropriately.
* @see #setShortValue(short,short)
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short getShortValue(final short holder) {
return (short) getValue(holder);
@@ -194,6 +205,7 @@ public short getShortValue(final short holder) {
* @param holder The int data containing the bits we're interested in.
* @return The selected bits, shifted right appropriately.
* @see #setValue(int,int)
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int getValue(final int holder) {
return getRawValue(holder) >>> shiftCount;
@@ -215,6 +227,24 @@ public long getValue(final long holder) {
return getRawValue(holder) >>> shiftCount;
}
+ /**
+ * Verifies that this field's mask fits in an {@code int} holder before an
{@code int}, {@code short} or {@code byte} accessor uses it.
+ * <p>
+ * Without this check, a mask with bits above bit 31 makes the narrow
accessors silently wrong: the {@code int} shift count is truncated mod 32, and a
+ * negative narrow holder is sign-extended to 64 bits before the {@code
long} mask is applied, reporting above-bit-31 flags as set even though the
holder
+ * type cannot contain them.
+ * </p>
+ *
+ * @return the mask, guaranteed to fit in 32 bits.
+ * @throws IllegalStateException if the mask has bits set above bit 31.
+ */
+ private long intMask() {
+ if (mask >>> Integer.SIZE != 0) {
+ throw new IllegalStateException("BitField mask 0x" +
Long.toHexString(mask) + " exceeds 32 bits; use the long accessors for this
field.");
+ }
+ return mask;
+ }
+
/**
* Tests whether all of the bits are set or not.
* <p>
@@ -223,9 +253,11 @@ public long getValue(final long holder) {
*
* @param holder The int data containing the bits we're interested in.
* @return {@code true} if all of the bits are set, else {@code false}.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public boolean isAllSet(final int holder) {
- return (holder & mask) == mask;
+ final long intMask = intMask();
+ return (holder & intMask) == intMask;
}
/**
@@ -251,9 +283,10 @@ public boolean isAllSet(final long holder) {
*
* @param holder The int data containing the bits we're interested in
* @return {@code true} if any of the bits are set, else {@code false}
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public boolean isSet(final int holder) {
- return (holder & mask) != 0;
+ return (holder & intMask()) != 0;
}
/**
@@ -276,9 +309,10 @@ public boolean isSet(final long holder) {
*
* @param holder The int data containing the bits we're interested in.
* @return The value of holder with the specified bits set to {@code 1}.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int set(final int holder) {
- return (int) (holder | mask);
+ return (int) (holder | intMask());
}
/**
@@ -298,6 +332,7 @@ public long set(final long holder) {
* @param holder The int data containing the bits we're interested in.
* @param flag indicating whether to set or clear the bits.
* @return The value of holder with the specified bits set or cleared.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int setBoolean(final int holder, final boolean flag) {
return flag ? set(holder) : clear(holder);
@@ -320,6 +355,7 @@ public long setBoolean(final long holder, final boolean
flag) {
*
* @param holder The byte data containing the bits we're interested in
* @return The value of holder with the specified bits set to {@code 1}
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public byte setByte(final byte holder) {
return (byte) set(holder);
@@ -331,6 +367,7 @@ public byte setByte(final byte holder) {
* @param holder The byte data containing the bits we're interested in.
* @param flag indicating whether to set or clear the bits.
* @return The value of holder with the specified bits set or cleared.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public byte setByteBoolean(final byte holder, final boolean flag) {
return flag ? setByte(holder) : clearByte(holder);
@@ -341,6 +378,7 @@ public byte setByteBoolean(final byte holder, final boolean
flag) {
*
* @param holder The short data containing the bits we're interested in.
* @return The value of holder with the specified bits set to {@code 1}.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short setShort(final short holder) {
return (short) set(holder);
@@ -352,6 +390,7 @@ public short setShort(final short holder) {
* @param holder The short data containing the bits we're interested in.
* @param flag indicating whether to set or clear the bits.
* @return The value of holder with the specified bits set or cleared.
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short setShortBoolean(final short holder, final boolean flag) {
return flag ? setShort(holder) : clearShort(holder);
@@ -364,6 +403,7 @@ public short setShortBoolean(final short holder, final
boolean flag) {
* @param value The new value for the specified bits
* @return The value of holder with the bits from the value parameter
replacing the old bits
* @see #getShortValue(short)
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public short setShortValue(final short holder, final short value) {
return (short) setValue(holder, value);
@@ -376,9 +416,11 @@ public short setShortValue(final short holder, final short
value) {
* @param value The new value for the specified bits.
* @return The value of holder with the bits from the value parameter
replacing the old bits.
* @see #getValue(int)
+ * @throws IllegalStateException if this field's mask has bits set above
bit 31 (only possible via {@link #BitField(long)}) and so cannot be represented
in this holder type.
*/
public int setValue(final int holder, final int value) {
- return (int) (holder & ~mask | value << shiftCount & mask);
+ final long intMask = intMask();
+ return (int) (holder & ~intMask | value << shiftCount & intMask);
}
/**
diff --git a/src/test/java/org/apache/commons/lang3/BitFieldLongTest.java
b/src/test/java/org/apache/commons/lang3/BitFieldLongTest.java
index ad2c19a24..442c7dffe 100644
--- a/src/test/java/org/apache/commons/lang3/BitFieldLongTest.java
+++ b/src/test/java/org/apache/commons/lang3/BitFieldLongTest.java
@@ -19,6 +19,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import org.junit.jupiter.api.Test;
@@ -68,8 +69,9 @@ void testClearInt() {
assertEquals(BF_MULTI.clear(-1), 0xFFFF_C07F);
assertEquals(BF_SINGLE.clear(-1), 0xFFFF_BFFF);
assertEquals(BF_ZERO.clear(-1), 0xFFFF_FFFF);
- assertEquals(BF_MULTI_L.clear(-1), 0xFFFF_FFFF);
- assertEquals(BF_SINGLE_L.clear(-1), 0xFFFF_FFFF);
+ // Above-bit-31 masks fail loudly on int holders instead of silently
no-op'ing (see BitField#intMask()).
+ assertThrows(IllegalStateException.class, () -> BF_MULTI_L.clear(-1));
+ assertThrows(IllegalStateException.class, () -> BF_SINGLE_L.clear(-1));
assertEquals(BF_ZERO_L.clear(-1), 0xFFFF_FFFF);
}
@@ -94,8 +96,9 @@ void testClearShort() {
assertEquals(BF_MULTI.clearShort((short) - 1), (short) 0xC07F);
assertEquals(BF_SINGLE.clearShort((short) - 1), (short) 0xBFFF);
assertEquals(BF_ZERO.clearShort((short) -1), (short) 0xFFFF);
- assertEquals(BF_MULTI_L.clearShort((short) - 1), (short) 0xFFFF);
- assertEquals(BF_SINGLE_L.clearShort((short) - 1), (short) 0xFFFF);
+ // Above-bit-31 masks fail loudly on short holders instead of silently
no-op'ing (see BitField#intMask()).
+ assertThrows(IllegalStateException.class, () ->
BF_MULTI_L.clearShort((short) -1));
+ assertThrows(IllegalStateException.class, () ->
BF_SINGLE_L.clearShort((short) -1));
assertEquals(BF_ZERO_L.clearShort((short) -1), (short) 0xFFFF);
}
@@ -121,11 +124,12 @@ void testGetRawValue() {
assertEquals(BF_SINGLE.getRawValue(0), 0);
assertEquals(BF_ZERO.getRawValue(-1), 0);
assertEquals(BF_ZERO.getRawValue(0), 0);
- // mask > max int and int input
- assertEquals(BF_MULTI_L.getRawValue(-1), 0);
- assertEquals(BF_MULTI_L.getRawValue(0), 0);
- assertEquals(BF_SINGLE_L.getRawValue(-1), 0);
- assertEquals(BF_SINGLE_L.getRawValue(0), 0);
+ // mask > max int and int input: fails loudly (the old silent 0 was
incoherent with the
+ // sign-extended answers the same mixture produced in isSet/isAllSet).
+ assertThrows(IllegalStateException.class, () ->
BF_MULTI_L.getRawValue(-1));
+ assertThrows(IllegalStateException.class, () ->
BF_MULTI_L.getRawValue(0));
+ assertThrows(IllegalStateException.class, () ->
BF_SINGLE_L.getRawValue(-1));
+ assertThrows(IllegalStateException.class, () ->
BF_SINGLE_L.getRawValue(0));
assertEquals(BF_ZERO_L.getRawValue(-1), 0);
assertEquals(BF_ZERO_L.getRawValue(0), 0);
// mask > max int and long input
@@ -149,11 +153,11 @@ void testGetShortRawValue() {
assertEquals(BF_SINGLE.getShortRawValue((short) 0), (short) 0);
assertEquals(BF_ZERO.getShortRawValue((short) -1), (short) 0);
assertEquals(BF_ZERO.getShortRawValue((short) 0), (short) 0);
- // mask > max int and short input
- assertEquals(BF_MULTI_L.getShortRawValue((short) - 1), (short) 0);
- assertEquals(BF_MULTI_L.getShortRawValue((short) 0), (short) 0);
- assertEquals(BF_SINGLE_L.getShortRawValue((short) - 1), (short) 0);
- assertEquals(BF_SINGLE_L.getShortRawValue((short) 0), (short) 0);
+ // mask > max int and short input: fails loudly (see testGetRawValue).
+ assertThrows(IllegalStateException.class, () ->
BF_MULTI_L.getShortRawValue((short) -1));
+ assertThrows(IllegalStateException.class, () ->
BF_MULTI_L.getShortRawValue((short) 0));
+ assertThrows(IllegalStateException.class, () ->
BF_SINGLE_L.getShortRawValue((short) -1));
+ assertThrows(IllegalStateException.class, () ->
BF_SINGLE_L.getShortRawValue((short) 0));
assertEquals(BF_ZERO_L.getShortRawValue((short) -1), (short) 0);
assertEquals(BF_ZERO_L.getShortRawValue((short) 0), (short) 0);
}
diff --git a/src/test/java/org/apache/commons/lang3/BitFieldTest.java
b/src/test/java/org/apache/commons/lang3/BitFieldTest.java
index 784acb8f5..4624d7436 100644
--- a/src/test/java/org/apache/commons/lang3/BitFieldTest.java
+++ b/src/test/java/org/apache/commons/lang3/BitFieldTest.java
@@ -19,6 +19,7 @@
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import org.junit.jupiter.api.Test;
@@ -305,4 +306,41 @@ void testSetValue() {
// verify that excess bits are stripped off
assertEquals(BF_SINGLE.setValue(0x4000, 2), 0);
}
+
+ /**
+ * Tests that a mask wider than 32 bits (only constructible via {@link
BitField#BitField(long)}) makes every int/short/byte holder accessor fail loudly
+ * instead of silently answering wrongly, and that a long-constructed mask
that fits in 32 bits keeps working with the narrow accessors.
+ */
+ @Test
+ void testWideMaskRejectsNarrowHolderAccessors() {
+ final BitField wide = new BitField(0x100000000L);
+ // Before the guard, sign extension made every above-bit-31 flag read
as set on any negative int holder.
+ assertThrows(IllegalStateException.class, () -> wide.isSet(-1));
+ assertThrows(IllegalStateException.class, () -> wide.isAllSet(-1));
+ assertThrows(IllegalStateException.class, () -> wide.getValue(-1));
+ assertThrows(IllegalStateException.class, () -> wide.getRawValue(-1));
+ assertThrows(IllegalStateException.class, () ->
wide.getShortValue((short) -1));
+ assertThrows(IllegalStateException.class, () ->
wide.getShortRawValue((short) -1));
+ assertThrows(IllegalStateException.class, () -> wide.clear(-1));
+ assertThrows(IllegalStateException.class, () -> wide.clearByte((byte)
-1));
+ assertThrows(IllegalStateException.class, () ->
wide.clearShort((short) -1));
+ assertThrows(IllegalStateException.class, () -> wide.set(0));
+ assertThrows(IllegalStateException.class, () -> wide.setBoolean(0,
true));
+ assertThrows(IllegalStateException.class, () -> wide.setByte((byte)
0));
+ assertThrows(IllegalStateException.class, () ->
wide.setByteBoolean((byte) 0, true));
+ assertThrows(IllegalStateException.class, () -> wide.setShort((short)
0));
+ assertThrows(IllegalStateException.class, () ->
wide.setShortBoolean((short) 0, true));
+ assertThrows(IllegalStateException.class, () ->
wide.setShortValue((short) 0, (short) 1));
+ assertThrows(IllegalStateException.class, () -> wide.setValue(0, 1));
+ // The long accessors remain fully functional for the same field.
+ assertFalse(wide.isSet(0xFFFFFFFFL));
+ assertTrue(wide.isSet(0x100000000L));
+ assertEquals(1L, wide.getValue(0x100000000L));
+ assertEquals(0x100000000L, wide.set(0L));
+ // A long-constructed mask that fits in 32 bits keeps working with the
narrow accessors.
+ final BitField narrow = new BitField(0x80000000L);
+ assertTrue(narrow.isSet(-1));
+ assertEquals(1, narrow.getValue(-1));
+ assertEquals(0x80000000, narrow.set(0));
+ }
}
