mozilla / rhino

Rhino is an open-source implementation of JavaScript written entirely in Java
https://rhino.github.io
Other
4.12k stars 848 forks source link

Code to fix the Reflective Access (IllegalAccessException) on private classes of a module is a bit to strict #1269

Open jcompagner opened 1 year ago

jcompagner commented 1 year ago

We upgraded to the latest release of rhino and i did see that rhino did had some patches to get around the problem of accessing stuff of modules that are not public api of that module. We had a patch for that in our older rhino fork, i discussed this already some years ago. And i did notice that rhino had its own thing so i dropped my patch. But then the problem came back like:

Error during evaluation:Wrapped java.lang.IllegalAccessException: class org.mozilla.javascript.MemberBox cannot access class sun.java2d.SunGraphics2D (in module java.desktop) because module java.desktop does not export sun.java2d to unnamed module @57839834

i checked it and the fix is easy:

https://github.com/Servoy/rhino/commit/49c4a01226b548bf9796f509c223a7aa72aea6eb

The problem is that rhino now checks for if the class that comes in is public, and if not it directly returns null But the private class like sun.java2d.SunGraphics2D can be really a private/package scoped class, that doesn't have to be public. So the public check should just be removed and we should just try to find an interface or its superclass that has to be public then to be able to access it.

But with the current implementation it is not search over at all because the first check already says "not public"

p-bakker commented 1 year ago

The earlier discussion: https://github.com/mozilla/rhino/issues/462#issuecomment-823107173

And the PR with the changes in this area that went into v1.7.14 #1040

I see in https://github.com/mozilla/rhino/issues/462#issuecomment-406085895 that @gbrail suggested as one possible solution to make changes to MemberBox, but that ended up not happening in #1040

https://github.com/Servoy/rhino/commit/49c4a01226b548bf9796f509c223a7aa72aea6eb however does make changes to MemberBox

Not sure at all if that's the right approach, but regardless, to be able to move forward, please provide a PR including a UnitTest covering the changes for the committers to look at

jcompagner commented 1 year ago

This issue is not really about that you want to call that.. but avoid calling it at all on private classes..

Because rhino shouldn't call it on the actual return type class (which is private) but on a public interface that that private class implements or a public base class that it extends

That code is in rhino that does that, it's only a bit to strict, 1 outer if needs to be removed

jcompagner commented 1 year ago

this fix (just removing that if for that ispublic check) was not enough to fix this i also needed another patch:

https://github.com/Servoy/rhino/commit/0fd1ec10b4a2f35b4ca9915b83f36f93b0d7837f

else for example code like this:

var fsize = 11;
        var ft = java.awt.Font.PLAIN;
        if (fontWeight > '700' ) {
            ft = java.awt.Font.BOLD;
        }
        var font = new Packages.java.awt.Font(fontName, ft, fsize);
        var grapicConfig =
Packages.java.awt.GraphicsEnvironment.getLocalGraphicsEnvironment().getDefaultScreenDevice().getDefaultConfiguration();
        var canvas = new Packages.java.awt.Canvas(grapicConfig);
        var w = canvas.getFontMetrics(font).stringWidth(text);

        return w;

will fail because the subclass of the starting class is still also not accessible and you need to go one higher up And this can really only be tested by using canAccess which is a Java11 api (or just just have to call in a loop the method and catch all the exception until you have one)

p-bakker commented 2 months ago

Rhino is now Java 11 or higher. A PR still welcome for this