[Hippo]

Patches a bug when parsing oak class files (versions <= 45.2)

The code attribute's maxStack, maxLocals, codeLength is half the size (u2 -> u1, u4 -> u2)

[Hippo]

Note: All test pass with mvn clean verify

[garydgregory]

-1 for now:

• Fails the build due to a checkstyle error: https://travis-ci.org/github/apache/commons-bcel/jobs/716255992
• No unit tests. One or more unit tests should fail without the patch to the main folder.
• The implementation feels at first glance like a class file version specific brute force hack using conditionals and booleans, as opposed to a more object oriented solution. Needs more study on my side as well.

[Hippo]

I will look into the check style error, and ill add a unit test. But as far as it being a version specific brute force hack is well, because this only occurs when one condition is met.

Take a look here
https://github.com/ItzSomebody/openjdk-jdk8u/blob/e87709def542f064a7ab9fa75542230e40876310/hotspot/src/share/vm/classfile/classFileParser.cpp#L2137

But if you really want me to give an oop approach on it I will do so.

[garydgregory]

I think we should also have a class file or jar file that we can run through other existing tests.

[Hippo]

Since this is just support for oak class files, do I really need to write a test for it? Can't I just use the existing test cases?

[Hippo]

OakFile.class.zip

This is an oak class file that BCEL fails to parse without my patches, I fixed the checkstyle error. But I am still not sure how I feel about over complicating things with an oop system on weather to read a u1 or u2. But if that is what you want I will implement it once I get off work 👍

[Hippo]

Actually I just thought of a more stable way to implement this, I will do so when I get the chance.

[garydgregory]

OakFile.class.zip

This is an oak class file that BCEL fails to parse without my patches, I fixed the checkstyle error. But I am still not sure how I feel about over complicating things with an oop system on weather to read a u1 or u2. But if that is what you want I will implement it once I get off work 👍

Thank you for the class file. I think we'll need the source to include it in the project repo to make sure the licensing is OK for an Apache project.

[Hippo]

OakFile.java.zip

There is no license, it's practically a hello world program.

[Hippo]

I realized that my idea for making it "more stable" would require a massive overhaul on the library, which doesn't seem like a good idea. I can think of a different approach if you would like, though I personally don't think its necessary.

[garydgregory]

OakFile.java.zip

There is no license, it's practically a hello world program.

Hi @Hippo
I'll should be able to take a look this weekend to see if a cleaner approach can be devised. In the meantime, please provide the source .java and instructions on how you created the class file if it is out of the ordinary Java tooling we are all used to. Is there an Oak/Java 1.0 compiler you used we can download to create additional files?

[garydgregory]

Notes
From: https://docs.oracle.com/javase/specs/jvms/se6/html/ClassFile.doc.html footnote 1:

The Java virtual machine implementation of Sun's JDK release 1.0.2 supports class file format versions 45.0 through 45.3 inclusive. Sun's JDK releases 1.1.X can support class file formats of versions in the range 45.0 through 45.65535 inclusive. Implementations of version 1.2 of the Java 2 platform can support class file formats of versions in the range 45.0 through 46.0 inclusive.

[garydgregory]

Should the check be minor <= 3 instead? See notes.
Are you saying that 45.2 is not compatible with 45.3?

[Hippo]

OakFile.java.zip
There is no license, it's practically a hello world program.

Hi @Hippo
I'll should be able to take a look this weekend to see if a cleaner approach can be devised. In the meantime, please provide the source .java and instructions on how you created the class file if it is out of the ordinary Java tooling we are all used to. Is there an Oak/Java 1.0 compiler you used we can download to create additional files?

No I don't have an oak compiler, but you can emulate one by transforming a class file and changing its version to a proper oak version and halfing the correct data types in the code attribute.

[Hippo]

Should the check be minor <= 3 instead? See notes.
Are you saying that 45.2 is not compatible with 45.3?

No it should be minor < 3 take a look at the jvm's class parser
https://github.com/AdoptOpenJDK/openjdk-jdk8u/blob/4b69552faa8de8f7de5bed1c95c12c025702b8ee/hotspot/src/share/vm/classfile/classFileParser.cpp#L2137

Also I check major < Const.MAJOR_1_1 just in-case the major version is less than 45, which shouldn't happen, but it's there just in-case.

[Hippo]

Any updates?

[garydgregory]

@Hippo Still no unit test or test resource that demonstrates the actual issue.

[garydgregory]

Closing: My current test shows we can parse Java 1.1 JAR files:

mvn clean test -Dtest=JdkGenericDumpTest -DExtraJavaHomes='/Users/garygregory/test/jdk-1_1_8_16-solaris-i586/' -Dbcel.print.JarNames -Dbcel.print.ClassNames

[Hippo]

@garydgregory I have added a test for what this supposed to patch. This is for versions older than 1.1 so that's why your test passed.

60ae991 introduces the test in ClassParserTest as well as a sample class file. The test fails, once the patches from ea802f5 are integrated then it passes.

[garydgregory]

@Hippo

Thank you for your update. Please run a local build (mvn by itself), BEFORE you push and fix issues, otherwise, you're wasting runner resources and maintainers' time.

[Hippo]

@garydgregory My apologies, latest commit fixes the checkstyle issue.

[garydgregory]

@Hippo
Thanks for working on BCEL-341. The historical compact Code header is a real compatibility issue, but I think the patch needs a few changes before it is ready to merge, so I created PR 542, you are credited in changes.xml.

Notes on this PR.

• Preserve existing parsing behavior. Modern JVMs also accept versions 45.0 -> 45.2 with standard Code headers. Selecting the compact layout solely from the version breaks files BCEL could previously parse. An explicit parsing option, with the standard layout remaining the default, would preserve compatibility.
• Cover code generation and version changes. The header setting needs to survive copying and regeneration through MethodGen and BCELifier. Dumping a class must also select a consistent layout, including when its version changes to 45.3 or later.
• Correct the test fixture. The added fixture declares max_locals = 0 for main(String[]), which requires at least one local-variable slot for its argument. The fixture should be loadable by a JVM that supports its format.
• Expand regression coverage. Besides parsing and dumping, tests should exercise standard headers with older version numbers, field-width limits, copying, code generation, and class-version changes.
  The Javadoc should also describe exactly which fields change width and how the layout is selected.

Closing in favor of PR 542