Bug 63781 - Prevent illegal reflective access warnings / errors from BeanELResolver
Summary: Prevent illegal reflective access warnings / errors from BeanELResolver
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: EL (show other bugs)
Version: 9.0.26
Hardware: PC All
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-29 12:15 UTC by stefan.ocke
Modified: 2019-10-05 08:35 UTC (History)
0 users



Attachments
Draft patch (8.16 KB, patch)
2019-10-03 14:09 UTC, Mark Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description stefan.ocke 2019-09-29 12:15:02 UTC
With Java module system, there might be cases, where interfaces are exported by a module, but not their implementations.
In such cases BeanELResolver will currently cause warnings / errors like this:

[ERROR] java.lang.IllegalAccessException: class javax.el.BeanELResolver cannot access class C (in module M) because module M does not export <packagfe of C> to unnamed module ...

This could, and should be prevented, since in later Java versions those warnings will become errors by default.

There already seems to be the right spot in code to achieve this. In javax.el.Util (which seems to be tomcat-EL-sepcific, despite its package name) there is the following code in getMethod:

if (m == null || Modifier.isPublic(type.getModifiers())) {
            return m;
        }
        Class<?>[] inf = type.getInterfaces();
        Method mp = null;
        for (int i = 0; i < inf.length; i++) {
            try {
                mp = inf[i].getMethod(m.getName(), m.getParameterTypes());
                mp = getMethod(mp.getDeclaringClass(), mp);
...

AFAIU, that code checks, if the type that declares the method, is public.
If not, it looks for the same method in interfaces and superclasses.

The check for "is public" could be extended to "and is exported by the module".
Comment 1 Mark Thomas 2019-10-03 14:09:05 UTC
Created attachment 36807 [details]
Draft patch

The fix for this is rather more involved than it might first appear.

The check in Java is AccessibleObject.canAccess(Object) but that is a new method in Java 9 and the EL API (and implementation) have to work on Java 8. That means introducing a cut-down version of JreCompat into the EL API.

There are a further 9 places in EL API where Modifiers.isPublic() is called. Each of those also need to be checked to see if a similar change is required there. As do the 7 places it is called elsewhere in the Tomcat code base.

I've attached a draft patch that addresses the specific issue raised in this report for review / comment. I intend to use this as the basis for a broader, more complete patch.
Comment 2 stefan.ocke 2019-10-04 08:46:14 UTC
Thank you for considering this. Sorry, I did not want to imply it was easy... 

I has a look at you patch and it looks reasonable to me.

Maybe, in Jre9Compat there could be a check for isPublic at first. If not, return false immediately, if it is public, do the (reflective) canAccess call.
Just for performance ...
Comment 3 Mark Thomas 2019-10-04 08:57:46 UTC
I think you highlight a valid point. The patch conflates the test for the type being public and the new Java 9 accessibility. I think those two should be kept separate.

I have a fix locally for most of these issues. I'll re-work it to address the issue above before committing.
Comment 4 Mark Thomas 2019-10-04 16:58:21 UTC
Fixed in:
- master for 9.0.27 onwards
- 8.5.x for 8.5.47 onwards
- 7.0.x for 7.0.97 onwards
Comment 5 stefan.ocke 2019-10-05 07:53:47 UTC
I tried with 9.0-SNAPSHOT now.

Unfortunately, the problem ist still there and now I have twice as muc warnings as before:

WARNING: Illegal reflective access by javax.el.Jre9Compat (file:/C:/java/maven/repository/org/apache/tomcat/embed/tomcat-embed-el/9.0-SNAPSHOT/tomcat-embed-el-9.0-SNAPSHOT.jar) to method ...

WARNING: Illegal reflective access by javax.el.BeanELResolver (file:/C:/java/maven/repository/org/apache/tomcat/embed/tomcat-embed-el/9.0-SNAPSHOT/tomcat-embed-el-9.0-SNAPSHOT.jar) to method ...

I am not yet sure what is going on here, but I will investigate further.
Comment 6 stefan.ocke 2019-10-05 08:05:44 UTC
There is probably something wrong with the warnings in the JDK (?)

Since if I set  

--illegal-access=deny

it works, where it would have failed before with 9.0.26.

However, with 

--illegal-access=warn

all the (fixed) warnings appear.

Strange.
Comment 7 stefan.ocke 2019-10-05 08:35:21 UTC
Okay, the explanation is:

With --illegal-access=warn , the new canAccess()-check will return true, since the access is still allowed. It will, however, log the warning (which might be a little bit verbose, since we want to CHECK the access, but not do it).
Since canAccess returned true, findMethod() will still return the method from the un-exported module. This leads to the second warning.

With --illegal-access=deny, the new canAccess()-check will return false and not log anything. 
Then findMethod() will search further for an exported method.

I don't think there is anything more than can be done here.
The warnings will persist, but they will never turn into errors. And that is the primary goal.
(Maybe there should be some hint in the documentation about this to avoid further bug tickets)