This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-bcel.git
The following commit(s) were added to refs/heads/master by this push:
new 4619e7bb Correct handling of references to unused/invalid constant
pool entries
4619e7bb is described below
commit 4619e7bb0ea61f82a4b551ebd8156b8841e5ecf2
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Dec 13 14:54:55 2022 +0000
Correct handling of references to unused/invalid constant pool entries
Ensure that references to the unused constant pool entry after a
long/double entry triggers a ClassFormatException, not a
NullPointerException.
---
src/changes/changes.xml | 1 +
.../org/apache/bcel/classfile/ConstantPool.java | 12 ++++---
src/test/java/org/apache/bcel/OssFuzzTestCase.java | 5 +++
.../bcel/classfile/ConstantPoolTestCase.java | 40 +++++++++++++--------
src/test/resources/ossfuzz/issue54254/Test.class | Bin 0 -> 42 bytes
5 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 611c1bf1..b03e0f4a 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -67,6 +67,7 @@ The <action> type attribute can be add,update,fix,remove.
<!-- FIX -->
<action type="fix" dev="markt" due-to="OSS-Fuzz">When
parsing an class with an invalid constant reference, ensure ClassParser.parse()
throws ClassFormatException, not NullPointerException.</action>
<action type="fix" dev="markt" due-to="OSS-Fuzz">Ensure
that references to a constant pool entry with index zero trigger a
ClassFormatException, not a NullPointerException.</action>
+ <action type="fix" dev="markt" due-to="OSS-Fuzz">Ensure
that references to the unused constant pool entry after a long/double entry
triggers a ClassFormatException, not a NullPointerException.</action>
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Gary
Gregory">Bump commons-parent from 54 to 55 #189.</action>
</release>
diff --git a/src/main/java/org/apache/bcel/classfile/ConstantPool.java
b/src/main/java/org/apache/bcel/classfile/ConstantPool.java
index e5ee1bcc..e94f35dd 100644
--- a/src/main/java/org/apache/bcel/classfile/ConstantPool.java
+++ b/src/main/java/org/apache/bcel/classfile/ConstantPool.java
@@ -308,13 +308,17 @@ public class ConstantPool implements Cloneable, Node,
Iterable<Constant> {
throw new ClassFormatException("Invalid constant pool reference at
index: " + index +
". Expected " + castTo + " but was " +
constantPool[index].getClass());
}
+ if (index > 1) {
+ final Constant prev = constantPool[index - 1];
+ if (prev != null && (prev.getTag() == Const.CONSTANT_Double ||
prev.getTag() == Const.CONSTANT_Long)) {
+ throw new ClassFormatException("Constant pool at index " +
index + " is invalid. The index is unused due to the preceeding "
+ + Const.getConstantName(prev.getTag()) + ".");
+ }
+ }
// Previous check ensures this won't throw a ClassCastException
final T c = castTo.cast(constantPool[index]);
if (c == null) {
- final Constant prev = constantPool[index - 1];
- if (prev == null || prev.getTag() != Const.CONSTANT_Double &&
prev.getTag() != Const.CONSTANT_Long) {
- throw new ClassFormatException("Constant pool at index " +
index + " is null.");
- }
+ throw new ClassFormatException("Constant pool at index " + index +
" is null.");
}
return c;
}
diff --git a/src/test/java/org/apache/bcel/OssFuzzTestCase.java
b/src/test/java/org/apache/bcel/OssFuzzTestCase.java
index c82b849a..e71c2c75 100644
--- a/src/test/java/org/apache/bcel/OssFuzzTestCase.java
+++ b/src/test/java/org/apache/bcel/OssFuzzTestCase.java
@@ -71,6 +71,11 @@ public class OssFuzzTestCase {
testOssFuzzReproducer("54119");
}
+ @Test
+ public void testIssue54254() throws Exception {
+ testOssFuzzReproducer("54254");
+ }
+
private void testOssFuzzReproducer(final String issue) throws Exception {
final File reproducerFile = new
File("target/test-classes/ossfuzz/issue" + issue + "/Test.class");
try (final FileInputStream reproducerInputStream = new
FileInputStream(reproducerFile)) {
diff --git a/src/test/java/org/apache/bcel/classfile/ConstantPoolTestCase.java
b/src/test/java/org/apache/bcel/classfile/ConstantPoolTestCase.java
index 8d0b20bb..0f0e3658 100644
--- a/src/test/java/org/apache/bcel/classfile/ConstantPoolTestCase.java
+++ b/src/test/java/org/apache/bcel/classfile/ConstantPoolTestCase.java
@@ -17,14 +17,12 @@
package org.apache.bcel.classfile;
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.fail;
import java.io.IOException;
-import java.util.stream.IntStream;
-
import org.apache.bcel.AbstractTestCase;
import org.apache.bcel.Const;
import org.apache.bcel.generic.ConstantPoolGen;
@@ -59,13 +57,19 @@ public class ConstantPoolTestCase extends AbstractTestCase {
assertEquals(1, fields.length);
assertEquals(ClassWithDoubleConstantPoolItem.class.getDeclaredFields()[0].getName(),
fields[0].getName());
final ConstantPool pool = c.getConstantPool();
- IntStream.range(1, pool.getLength()).forEach(i ->
assertDoesNotThrow(() -> {
- final Constant constant = pool.getConstant(i);
- if (constant instanceof ConstantDouble) {
- assertEquals(classWithDoubleConstantPoolItem.d,
((ConstantDouble) constant).getBytes());
+ for (int i = 1; i < pool.getLength(); i++) {
+ try {
+ final Constant constant = pool.getConstant(i);
+ if (constant instanceof ConstantDouble) {
+ assertEquals(classWithDoubleConstantPoolItem.d,
((ConstantDouble) constant).getBytes());
+ // Next constant pool entry will be invalid so skip it
+ i++;
+ }
+ } catch (Throwable t) {
+ t.printStackTrace();
+ fail();
}
- return constant;
- }));
+ }
}
}
@@ -79,13 +83,19 @@ public class ConstantPoolTestCase extends AbstractTestCase {
assertEquals(1, fields.length);
assertEquals(ClassWithLongConstantPoolItem.class.getDeclaredFields()[0].getName(),
fields[0].getName());
final ConstantPool pool = c.getConstantPool();
- IntStream.range(1, pool.getLength()).forEach(i ->
assertDoesNotThrow(() -> {
- final Constant constant = pool.getConstant(i);
- if (constant instanceof ConstantLong) {
- assertEquals(classWithLongConstantPoolItem.l,
((ConstantLong) constant).getBytes());
+ for (int i = 1; i < pool.getLength(); i++) {
+ try {
+ final Constant constant = pool.getConstant(i);
+ if (constant instanceof ConstantLong) {
+ assertEquals(classWithLongConstantPoolItem.l,
((ConstantLong) constant).getBytes());
+ // Next constant pool entry will be invalid so skip it
+ i++;
+ }
+ } catch (Throwable t) {
+ t.printStackTrace();
+ fail();
}
- return constant;
- }));
+ }
}
}
diff --git a/src/test/resources/ossfuzz/issue54254/Test.class
b/src/test/resources/ossfuzz/issue54254/Test.class
new file mode 100644
index 00000000..275741ee
Binary files /dev/null and b/src/test/resources/ossfuzz/issue54254/Test.class
differ
