Skip to content

Bug #45 - Reflective access with CGLIB breaks windowbuilder in java 16 - #55

Merged
lcaron merged 1 commit into
masterfrom
bugfix/Bug_#45_Reflective_access_with_CGLIB_breaks_windowbuilder_in_java_16_and_higher_
Oct 17, 2021
Merged

lcaron merged 1 commit into
masterfrom
bugfix/Bug_#45_Reflective_access_with_CGLIB_breaks_windowbuilder_in_java_16_and_higher_

Conversation

@lcaron

@lcaron lcaron commented Oct 17, 2021

Copy link
Copy Markdown
Contributor

I tried first to replace cglib by bytebuddy, and it is very very tricky, long and it could be a bug nest.

This PR is based on new approach, a trick to bypass module security (same trick in JDK 16 with the --illegal-access=permit argument). I found the solution at [1]

I use burningwave library [2] and the following snippet [3]

[1] = cglib/cglib#191
[2] = https://github.com/burningwave/core
[3] = https://github.com/JJBRT/advanced-java-tutorials/blob/master/reflection/src/main/java/com/github/jjbrt/reflection/AllModulesToAllModulesExporter.java

@lcaron
lcaron force-pushed the bugfix/Bug_#45_Reflective_access_with_CGLIB_breaks_windowbuilder_in_java_16_and_higher_ branch from 0243f6c to b08e934 Compare October 17, 2021 10:42
@lcaron
lcaron merged commit 052cf36 into master Oct 17, 2021
@lcaron
lcaron deleted the bugfix/Bug_#45_Reflective_access_with_CGLIB_breaks_windowbuilder_in_java_16_and_higher_ branch October 17, 2021 19:34
@merks

merks commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

Have you tried this with Java 17? I ask because in the Java 17 installation I have --illegal-access=permit not not work while --add-opens=java.base/java.lang=ALL-UNNAMED' does work. I know --illegal-access=permit` works for Java 16.

According to https://blogs.oracle.com/javamagazine/post/a-peek-into-java-17-continuing-the-drive-to-encapsulate-the-java-runtime-internals that option was removed and it points to https://openjdk.java.net/jeps/403 for reference.

@lcaron

lcaron commented Oct 18, 2021

Copy link
Copy Markdown
Contributor Author

Hi ed

Yes I have tested with JDK 17.

The previous bugfix consisted in adding this "illegal-access" parameter to the vm arguments of Eclipse, and this argument is no longer used in Java 17.

In this PR i used a "hack" that allows all modules to access all modules (using the library Burningwave)

@merks

merks commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

Ah, that sounds horrible but good. That being said, did you file a CQ for the use of this library?

@lcaron

lcaron commented Oct 18, 2021

Copy link
Copy Markdown
Contributor Author

Heu no... can you tell me how to do this request ?

@merks
merks requested a review from vogella October 18, 2021 07:45
@merks

merks commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

@vogella Please help Laurent with the processes around dependencies on third party libraries...

@merks

merks commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

And note that I must confess that the processes have changed over the years and I'm not really sure exactly the details of the latest incantation.

https://www.eclipse.org/projects/handbook/#ip
https://www.eclipse.org/projects/handbook/#ip-third-party

@vogella

vogella commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

@merks I'm not a good adviser for process steps, as I rarely involved in these and dislike these steps. IIRC the foundation was planning to get rid of CQ?

Anyway, @lcaron IIRC you have to open a CQ on https://dev.eclipse.org/ipzilla/ asking if a library can be used. You would upload the lib and the foundation ask an external company to run a security and IP check on it. If everything is fine, you get the right to use it.

@vogella

vogella commented Oct 18, 2021

Copy link
Copy Markdown
Contributor

Great thanks to @lcaron for working on this.

@lcaron

lcaron commented Oct 18, 2021

Copy link
Copy Markdown
Contributor Author

Thank you @vogella I'm gonna do it today

@lcaron lcaron added the 1.9.8 label Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants