Skip to content

Keep ConstantPool.getConstant(int) backward compatible with v6.5.0#157

Merged
garydgregory merged 5 commits intoapache:masterfrom
KengoTODA:keep-backward-compatible
Oct 16, 2022
Merged

Keep ConstantPool.getConstant(int) backward compatible with v6.5.0#157
garydgregory merged 5 commits intoapache:masterfrom
KengoTODA:keep-backward-compatible

Conversation

@KengoTODA
Copy link
Copy Markdown
Contributor

@KengoTODA KengoTODA commented Oct 15, 2022

Hello, thanks for keeping commons-bcel maintained! It surely helps the Java community a lot. 🙌
I'm using commons-bcel to maintain bytecode analysis tool like SpotBugs.

Today I found that BCEL v6.6.0 makes test cases in SpotBugs failed: spotbugs/spotbugs#2213

The reason is that, ConstantPool#getConstant(int) throws a ClassFormatException if constantPool[index] is null. But it is possible even when the class file is valid: you can verify it by making a simple class like below:

class ClassWithNullConstantPoolItem {
  double d = 42; // here is the key; we need a double constant value
}

Then javac ClassWithNullConstantPoolItem.java && javap -v ClassWithNullConstantPoolItem will print a constant pool like below:

Constant pool:
   #1 = Methodref          #6.#15         // java/lang/Object."<init>":()V
   #2 = Double             42.0d
   #4 = Fieldref           #5.#16         // ClassWithNullConstantPoolItem.d:D

We can confirm that there is no #3, and in such case, getConstant(3) becomes null.
In our repository, Issue389.java is one of such cases.

So in my opinion, it is not so good to throw ClassFormatException. Here I suggest rollback this change made in v6.6.0. Thanks for checking my PR!

@KengoTODA
fix: keep ConstantPool.getConstant(int) backward compatible with v6.5.0
99d3be3 
Signed-off-by: Kengo TODA <skypencil@gmail.com>
@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 15, 2022

full stack trace for reference 
The following errors occurred during analysis:
  Exception analyzing ghIssues.Issue389 using detector edu.umd.cs.findbugs.detect.FindOpenStream
    org.apache.bcel.classfile.ClassFormatException: Constant pool at index 11 is null.
      At org.apache.bcel.classfile.ConstantPool.getConstant(ConstantPool.java:307)
      At org.apache.bcel.classfile.ConstantPool.getConstant(ConstantPool.java:257)
      At edu.umd.cs.findbugs.detect.FindOpenStream.visitClassContext(FindOpenStream.java:304)
      At edu.umd.cs.findbugs.DetectorToDetector2Adapter.visitClass(DetectorToDetector2Adapter.java:76)
      At edu.umd.cs.findbugs.FindBugs2.lambda$analyzeApplication$1(FindBugs2.java:1108)
      At java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
      At edu.umd.cs.findbugs.CurrentThreadExecutorService.execute(CurrentThreadExecutorService.java:86)
      At java.base/java.util.concurrent.AbstractExecutorService.invokeAll(AbstractExecutorService.java:247)
      At edu.umd.cs.findbugs.FindBugs2.analyzeApplication(FindBugs2.java:1118)
      At edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:309)
      At edu.umd.cs.findbugs.test.AnalysisRunner.run(AnalysisRunner.java:115)
      At edu.umd.cs.findbugs.test.AnalysisRunner.run(AnalysisRunner.java:89)
      At edu.umd.cs.findbugs.AbstractIntegrationTest.performAnalysis(AbstractIntegrationTest.java:130)
      At edu.umd.cs.findbugs.ba.Issue389Test.test(Issue389Test.java:16)
      At java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      At java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
      At java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      At java.base/java.lang.reflect.Method.invoke(Method.java:568)
      At org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
      At org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
      At org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
      At org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
      At org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
      At org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
      At org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
      At org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
      At org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
      At org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
      At org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
      At org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
      At org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
      At org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
      At org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
      At org.junit.runners.ParentRunner.run(ParentRunner.java:413)
      At org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
      At org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
      At org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
      At org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
      At org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
      At jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
      At java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      At java.base/java.lang.reflect.Method.invoke(Method.java:568)
      At org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
      At org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
      At org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
      At org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
      At jdk.proxy2/jdk.proxy2.$Proxy5.processTestClass(Unknown Source)
      At org.gradle.api.internal.tasks.testing.worker.TestWorker$2.run(TestWorker.java:176)
      At org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:129)
      At org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:100)
      At org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:60)
      At org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
      At org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:133)
      At org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
      At worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
      At worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)

Note that there is another problem on the spotbugs side: SpotBugs iterates on the constant pool with for (int i = 0;, even though the first item in the constant pool is always null.

https://github.com/spotbugs/spotbugs/blob/838bf77781ab57b56fa159136635b9f8991df327/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindOpenStream.java#L303

But even after I applied a patch to replace it with for (int i = 1;, the build failed due to the problem I reported by this PR.

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 15, 2022 •
edited
Loading

Hi @KengoTODA
Thank you for your report.
You need a unit test that fails without your main changes, otherwise this will be a regression waiting to happen. Disabling a test makes for noise to maintain IMO.

There must be a better way to deal with this issue though because this PR will cause NPEs instead of a useful ClassFormatException message in all the BCEL call sites. Any ideas?

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 15, 2022 •
edited
Loading

I do not see what you are seeing, if I create a class org.apache.bcel.classfile.ClassWithNullConstantPoolItem with the same content as the one above and run javap I get:

javap -v target/test-classes/org/apache/bcel/classfile/ClassWithNullConstantPoolItem.class

Classfile /C:/Users/ggregory/git/a/commons-bcel/target/test-classes/org/apache/bcel/classfile/ClassWithNullConstantPoolItem.class
  Last modified Oct 15, 2022; size 414 bytes
  SHA-256 checksum dde65254ff1d0d604c9abee4a13dc34e31fc45c7fec0487e00ba6209cbb1e52d
  Compiled from "ClassWithNullConstantPoolItem.java"
public class org.apache.bcel.classfile.ClassWithNullConstantPoolItem
  minor version: 0
  major version: 52
  flags: (0x0021) ACC_PUBLIC, ACC_SUPER
  this_class: #1                          // org/apache/bcel/classfile/ClassWithNullConstantPoolItem
  super_class: #3                         // java/lang/Object
  interfaces: 0, fields: 1, methods: 1, attributes: 1
Constant pool:
   #1 = Class              #2             // org/apache/bcel/classfile/ClassWithNullConstantPoolItem
   #2 = Utf8               org/apache/bcel/classfile/ClassWithNullConstantPoolItem
   #3 = Class              #4             // java/lang/Object
   #4 = Utf8               java/lang/Object
   #5 = Utf8               d
   #6 = Utf8               D
   #7 = Utf8               <init>
   #8 = Utf8               ()V
   #9 = Utf8               Code
  #10 = Methodref          #3.#11         // java/lang/Object."<init>":()V
  #11 = NameAndType        #7:#8          // "<init>":()V
  #12 = Double             42.0d
  #14 = Fieldref           #1.#15         // org/apache/bcel/classfile/ClassWithNullConstantPoolItem.d:D
  #15 = NameAndType        #5:#6          // d:D
  #16 = Utf8               LineNumberTable
  #17 = Utf8               LocalVariableTable
  #18 = Utf8               this
  #19 = Utf8               Lorg/apache/bcel/classfile/ClassWithNullConstantPoolItem;
  #20 = Utf8               SourceFile
  #21 = Utf8               ClassWithNullConstantPoolItem.java
{
  double d;
    descriptor: D
    flags: (0x0000)

  public org.apache.bcel.classfile.ClassWithNullConstantPoolItem();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=1, args_size=1
         0: aload_0
         1: invokespecial #10                 // Method java/lang/Object."<init>":()V
         4: aload_0
         5: ldc2_w        #12                 // double 42.0d
         8: putfield      #14                 // Field d:D
        11: return
      LineNumberTable:
        line 20: 0
        line 21: 4
        line 20: 11
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0      12     0  this   Lorg/apache/bcel/classfile/ClassWithNullConstantPoolItem;
}
SourceFile: "ClassWithNullConstantPoolItem.java"

@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 15, 2022

Really interesting, it could be compiler dependent? I can reproduce it with AdoptOpenJDK-11.0.11+9.

I'll try to add a unit test to reproduce. Thanks.

@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 15, 2022

full javap result:

Classfile /private/tmp/ClassWithNullConstantPoolItem.class
  Last modified Oct 15, 2022; size 278 bytes
  MD5 checksum 1aa3daf4cf1fe494046af119c6e409b9
  Compiled from "ClassWithNullConstantPoolItem.java"
class ClassWithNullConstantPoolItem
  minor version: 0
  major version: 55
  flags: (0x0020) ACC_SUPER
  this_class: #5                          // ClassWithNullConstantPoolItem
  super_class: #6                         // java/lang/Object
  interfaces: 0, fields: 1, methods: 1, attributes: 1
Constant pool:
   #1 = Methodref          #6.#15         // java/lang/Object."<init>":()V
   #2 = Double             42.0d
   #4 = Fieldref           #5.#16         // ClassWithNullConstantPoolItem.d:D
   #5 = Class              #17            // ClassWithNullConstantPoolItem
   #6 = Class              #18            // java/lang/Object
   #7 = Utf8               d
   #8 = Utf8               D
   #9 = Utf8               <init>
  #10 = Utf8               ()V
  #11 = Utf8               Code
  #12 = Utf8               LineNumberTable
  #13 = Utf8               SourceFile
  #14 = Utf8               ClassWithNullConstantPoolItem.java
  #15 = NameAndType        #9:#10         // "<init>":()V
  #16 = NameAndType        #7:#8          // d:D
  #17 = Utf8               ClassWithNullConstantPoolItem
  #18 = Utf8               java/lang/Object
{
  double d;
    descriptor: D
    flags: (0x0000)

  ClassWithNullConstantPoolItem();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=3, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: aload_0
         5: ldc2_w        #2                  // double 42.0d
         8: putfield      #4                  // Field d:D
        11: return
      LineNumberTable:
        line 1: 0
        line 2: 4
}
SourceFile: "ClassWithNullConstantPoolItem.java"

@KengoTODA
test: add a test case that fails with v6.6.0
51bba27 
Signed-off-by: Kengo TODA <skypencil@gmail.com>
@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 16, 2022

Confirmed that this problem can be reproduced with Java version 8.0.345 and 17.0.4.1 distributed by Temurin. So it's not Java version dependent... are you using ecj or another compiler?

@KengoTODA
chore: stop using wildcard import
767760f 
Signed-off-by: Kengo TODA <skypencil@gmail.com>
@KengoTODA KengoTODA force-pushed the keep-backward-compatible branch from 170f5b2 to 767760f Compare October 16, 2022 00:24
@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 16, 2022

I was using Eclipse

garydgregory
garydgregory reviewed Oct 16, 2022
View reviewed changes
Comment thread src/test/java/org/apache/bcel/verifier/VerifyBadClassesTestCase.java Outdated
* BCEL-308: NullPointerException in Verifier Pass 3A
*/
@Test
@Disabled("ConstantPool item could be null even when class file is valid")
Copy link
Copy Markdown
Member

@garydgregory garydgregory Oct 16, 2022

Choose a reason for hiding this comment

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

Why is this disabled?

Copy link
Copy Markdown
Contributor Author

@KengoTODA KengoTODA Oct 16, 2022

Choose a reason for hiding this comment

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

This test failed after I removed the verification logic, but my new suggestion is enough to pass this case so I reverted this @Disabled annotation. 🤝

@KengoTODA
suggest a better fix instead of removing whole verification logic
bc0f1ac 
Signed-off-by: Kengo TODA <skypencil@gmail.com>
@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 16, 2022

I was using Eclipse

I see, then this problem should be OpenJDK-based compiler specific.

There must be a better way to deal with this issue though because this PR will cause NPEs instead of a useful ClassFormatException message in all the BCEL call sites. Any ideas?

I proposed bc0f1ac which throws an exception only when the class file format is really invalid. Could you review it when you have time? Thanks in advance!

@KengoTODA
minor fix
3e6f7f6 
Signed-off-by: Kengo TODA <skypencil@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 16, 2022 •
edited
Loading

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.46%. Comparing base (8defcc6) to head (3e6f7f6).
⚠️ Report is 1567 commits behind head on master.

Files with missing lines Patch % Lines
...n/java/org/apache/bcel/classfile/ConstantPool.java 25.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master     #157   +/-   ##
=========================================
  Coverage     44.45%   44.46%           
- Complexity     2518     2520    +2     
=========================================
  Files           362      362           
  Lines         15835    15838    +3     
  Branches       1988     1990    +2     
=========================================
+ Hits           7040     7042    +2     
  Misses         8090     8090           
- Partials        705      706    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@garydgregory garydgregory merged commit bc32885 into apache:master Oct 16, 2022
asfgit pushed a commit that referenced this pull request Oct 16, 2022
@garydgregory
Keep ConstantPool.getConstant(int) backward compatible with v6.5.0 #157.
463d3a7
@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 16, 2022

Hi @KengoTODA
Merged. How about fixing spotbugs/spotbugs#2191 ? ;-)

@KengoTODA KengoTODA deleted the keep-backward-compatible branch October 16, 2022 17:05
@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 16, 2022

I'll check it, thanks for reminding me!

@KengoTODA KengoTODA mentioned this pull request Oct 17, 2022
Closed
@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Oct 18, 2022 •
edited
Loading

note: I've checked the implementation of OpenJDK. I found the code that skips classfile entry after long constants and double constants, but cannot find its intention because the first commit already contains this implementation.

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Oct 18, 2022

@KengoTODA
Thank you for the update. Feel free to create a PR to improve the Javadocs or internal code comments. We need all the help we can get for maintenance ;-)

@KengoTODA KengoTODA mentioned this pull request Oct 23, 2022
Merged
1 task
@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Nov 15, 2022

Hello @KengoTODA
Would you be able to give an opinion on whether this PR is related to a regression we are seeing in Xalan?
The (long) thread: https://lists.apache.org/thread/dojn56v56chkytwffshm343rnj31x3kh
More specifically, we see errors like:

[xalantest]   ERROR  StylesheetTestlet{trax} boolean39.xsl threw: java.lang.VerifyError: (class: boolean39, method: template$dot$0 signature: (Lorg/apache/xalan/xsltc/DOM;Lorg/apache/xml/dtm/DTMAxisIterator;Lorg/apache/xml/serializer/SerializationHandler;I)V) Expecting to find double on stack
[xalantest] StylesheetTestlet boolean82.xsl
[xalantest] java.lang.VerifyError: (class: boolean82, method: template$dot$0 signature: (Lorg/apache/xalan/xsltc/DOM;Lorg/apache/xml/dtm/DTMAxisIterator;Lorg/apache/xml/serializer/SerializationHandler;I)V) Expecting to find double on stack
[xalantest] java.lang.VerifyError: (class: boolean82, method: template$dot$0 signature: (Lorg/apache/xalan/xsltc/DOM;Lorg/apache/xml/dtm/DTMAxisIterator;Lorg/apache/xml/serializer/SerializationHandler;I)V) Expecting to find double on stack
[xalantest]     at java.lang.Class.getDeclaredConstructors0(Native Method)
[xalantest]     at java.lang.Class.privateGetDeclaredConstructors(Class.java:2671)
[xalantest]     at java.lang.Class.getConstructor0(Class.java:3075)
[xalantest]     at java.lang.Class.newInstance(Class.java:412)
[xalantest]     at org.apache.xalan.xsltc.trax.TemplatesImpl.getTransletInstance(TemplatesImpl.java:340)
[xalantest]     at org.apache.xalan.xsltc.trax.TemplatesImpl.newTransformer(TemplatesImpl.java:369)
[xalantest]     at org.apache.xalan.xsltc.trax.TransformerFactoryImpl.newTransformer(TransformerFactoryImpl.java:619)
[xalantest]     at org.apache.qetest.xslwrapper.TraxSystemIdWrapper.transform(TraxSystemIdWrapper.java:168)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestlet.testDatalet(StylesheetTestlet.java:229)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestlet.execute(StylesheetTestlet.java:108)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestletDriver.processFileList(StylesheetTestletDriver.java:441)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestletDriver.processInputDir(StylesheetTestletDriver.java:397)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestletDriver.runTestCases(StylesheetTestletDriver.java:285)
[xalantest]     at org.apache.qetest.TestImpl.runTest(TestImpl.java:205)
[xalantest]     at org.apache.qetest.FileBasedTest.doMain(FileBasedTest.java:833)
[xalantest]     at org.apache.qetest.xsl.StylesheetTestletDriver.main(StylesheetTestletDriver.java:951)
[xalantest]     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[xalantest]     at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[xalantest]     at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[xalantest]     at java.lang.reflect.Method.invoke(Method.java:498)
[xalantest]     at org.apache.qetest.QetestUtils.main(QetestUtils.java:434)

@KengoTODA
Copy link
Copy Markdown
Contributor Author

KengoTODA commented Nov 16, 2022

Wmm, I don't think so.
This PR affects nothing related to iterating data in the class file, nor writing bytecode to class files...

BCEL 6.6.1 has no other big changes, but they tried with BCEL 6.6.2-SNAPSHOT, then it could be necessary to investigate rel/commons-bcel-6.6.1...master?

@garydgregory
Copy link
Copy Markdown
Member

garydgregory commented Nov 16, 2022

Thanks @KengoTODA
The regression found by Xalan is indeed unrelated to this PR. It is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garydgregory garydgregory garydgregory left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KengoTODA @garydgregory @codecov-commenter