You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/08/09 07:42:46 UTC
[GitHub] [commons-bcel] Hippo opened a new pull request #58: BCEL-341: Oak class file patch
Hippo opened a new pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671610099
[OakFile.class.zip](https://github.com/apache/commons-bcel/files/5053630/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 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on a change in pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on a change in pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#discussion_r468839745
##########
File path: src/main/java/org/apache/bcel/classfile/ClassParser.java
##########
@@ -303,5 +304,6 @@ private void readMethods() throws IOException, ClassFormatException {
private void readVersion() throws IOException, ClassFormatException {
minor = dataInputStream.readUnsignedShort();
major = dataInputStream.readUnsignedShort();
+ isOak = major < Const.MAJOR_1_1 || (major == Const.MAJOR_1_1 && minor < 3);
Review comment:
> > [OakFile.java.zip](https://github.com/apache/commons-bcel/files/5054556/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory edited a comment on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671911131
> [OakFile.java.zip](https://github.com/apache/commons-bcel/files/5054556/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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on a change in pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on a change in pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#discussion_r468840192
##########
File path: src/main/java/org/apache/bcel/classfile/ClassParser.java
##########
@@ -303,5 +304,6 @@ private void readMethods() throws IOException, ClassFormatException {
private void readVersion() throws IOException, ClassFormatException {
minor = dataInputStream.readUnsignedShort();
major = dataInputStream.readUnsignedShort();
+ isOak = major < Const.MAJOR_1_1 || (major == Const.MAJOR_1_1 && minor < 3);
Review comment:
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671700881
[OakFile.java.zip](https://github.com/apache/commons-bcel/files/5054556/OakFile.java.zip)
There is no license, it's practically a hello world program.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671654428
> [OakFile.class.zip](https://github.com/apache/commons-bcel/files/5053630/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671021527
Note: All test pass with mvn clean verify
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory edited a comment on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671932784
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671588995
I think we should also have a class file or jar file that we can run through other existing tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on a change in pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#discussion_r468562200
##########
File path: src/main/java/org/apache/bcel/classfile/ClassParser.java
##########
@@ -303,5 +304,6 @@ private void readMethods() throws IOException, ClassFormatException {
private void readVersion() throws IOException, ClassFormatException {
minor = dataInputStream.readUnsignedShort();
major = dataInputStream.readUnsignedShort();
+ isOak = major < Const.MAJOR_1_1 || (major == Const.MAJOR_1_1 && minor < 3);
Review comment:
Should the check be `minor <= 3` instead? See notes.
Are you saying that 45.2 is not compatible with 45.3?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671613683
Actually I just thought of a more stable way to implement this, I will do so when I get the chance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671710080
I realized that my idea for making it 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671586780
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-695178175
Any updates?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671412245
-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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671911131
> [OakFile.java.zip](https://github.com/apache/commons-bcel/files/5054556/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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on a change in pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#discussion_r468562200
##########
File path: src/main/java/org/apache/bcel/classfile/ClassParser.java
##########
@@ -303,5 +304,6 @@ private void readMethods() throws IOException, ClassFormatException {
private void readVersion() throws IOException, ClassFormatException {
minor = dataInputStream.readUnsignedShort();
major = dataInputStream.readUnsignedShort();
+ isOak = major < Const.MAJOR_1_1 || (major == Const.MAJOR_1_1 && minor < 3);
Review comment:
Should the check be `minor <= 3` instead? See notes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] garydgregory commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671932784
Notes
From: https://docs.oracle.com/javase/specs/jvms/se6/html/ClassFile.doc.html
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo edited a comment on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo edited a comment on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671710080
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on a change in pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on a change in pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#discussion_r468840192
##########
File path: src/main/java/org/apache/bcel/classfile/ClassParser.java
##########
@@ -303,5 +304,6 @@ private void readMethods() throws IOException, ClassFormatException {
private void readVersion() throws IOException, ClassFormatException {
minor = dataInputStream.readUnsignedShort();
major = dataInputStream.readUnsignedShort();
+ isOak = major < Const.MAJOR_1_1 || (major == Const.MAJOR_1_1 && minor < 3);
Review comment:
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-bcel] Hippo commented on pull request #58: BCEL-341: Oak class file patch
Posted by GitBox <gi...@apache.org>.
Hippo commented on pull request #58:
URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-671591550
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org