Bug 57132 - ImportHandler.resolveClass() fails to report conflicting import when called repeatedly
Summary: ImportHandler.resolveClass() fails to report conflicting import when called r...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: EL (show other bugs)
Version: 8.0.14
Hardware: PC All
: P2 minor (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-22 21:56 UTC by Konstantin Kolinko
Modified: 2014-10-25 16:23 UTC (History)
0 users



Attachments
2014-10-22_tc8_57132_testcases.patch (3.13 KB, patch)
2014-10-22 22:10 UTC, Konstantin Kolinko
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Kolinko 2014-10-22 21:56:39 UTC
I noted this issue when reviewing ImportHandler code after r1633440. I have test case that demonstrates it, but I do not have a fix yet.

javax.el.ImportHandler.resolveClass(String name) should throw an ELException if the same class can be ambiguously found in several packages.

To do so, it relies on calling findClass(className, true) and on "clazzes" field that is a cache of classes keyed by their "simple class names". A simple class name is the name without its package.

If resolveClass("foo") finds class foo in several packages then the first call to findClass() updates the clazzes cache and the second call finds the duplicate when trying to update the cache with a different class.

If resolveClass("foo") call is repeated, expected result is to report the error again. The actual result is that the first thing it does is to look into the cache. It finds the first class there and returns it without any errors.
Comment 1 Konstantin Kolinko 2014-10-22 22:10:21 UTC
Created attachment 32139 [details]
2014-10-22_tc8_57132_testcases.patch

Updated testcases for TestImportHandler to test that duplicate calls still report an error.

A comment in ImportHandler class shows my first idea on fixing this. If I remove first duplicate from the cache it fixes duplicate check in resolveClass() but breaks duplicate check in importClass().

I think that if I move duplicate removal from the cache into resolveClass() method itself, it will be fixed. But maybe there is a more clean solution.


BTW, there is no control that the class name argument in resolveClass() is actually a simple name without a package. JavaEE javadoc says that it should be simple name, but there is no control of that fact. JavaEE does not say whether it is ELException or IAE to be thrown if such a check were added. I think it is an ELException. [1]

[1] http://docs.oracle.com/javaee/7/api/javax/el/ImportHandler.html
Comment 2 Konstantin Kolinko 2014-10-23 08:48:26 UTC
The first (main) reported issue fixed by r1633769 and will be in Tomcat 8.0.15.

> 
> BTW, there is no control that the class name argument in resolveClass() is
> actually a simple name without a package. JavaEE javadoc says that it should
> be simple name, but there is no control of that fact. JavaEE does not say
> whether it is ELException or IAE to be thrown if such a check were added. I
> think it is an ELException. [1]
> 
> [1] http://docs.oracle.com/javaee/7/api/javax/el/ImportHandler.html

The above second issue is still pending. BTW, it can simply return "null" without reporting any errors.

Also noted the third issue:

ImportHandler.importClass() does not allow to import the exactly same class twice.

I think that such imports shall be silently swallowed. It boils down to adding a check whether conflicting Class instances are the same, (conflict == clazz).
Comment 3 Mark Thomas 2014-10-23 13:45:12 UTC
Reading the Javadoc for the second issue, I think returning null is a better response here. I've committed a couple of test cases and a fix for this.

Looking at the third issue now...
Comment 4 Mark Thomas 2014-10-23 13:50:35 UTC
Third issue fixed in 8.0.x for 8.0.15 onwards.
Comment 5 Christopher Schultz 2014-10-23 21:45:59 UTC
(In reply to Konstantin Kolinko from comment #2)
> Also noted the third issue:
> 
> ImportHandler.importClass() does not allow to import the exactly same class
> twice.
> 
> I think that such imports shall be silently swallowed. It boils down to
> adding a check whether conflicting Class instances are the same, (conflict
> == clazz).

I haven't read through the code to see how class object references are maintained, but I have been bitten in the past when Foo.class.getMethod("bar").getName() == Foo.class.getMethod("bar").getName() yields false.

Perhaps clazz.equals(conflict) would be better than == in this case?
Comment 6 Konstantin Kolinko 2014-10-25 16:23:41 UTC
(In reply to Christopher Schultz from comment #5)
> 
> I haven't read through the code to see how class object references are
> maintained, but I have been bitten in the past when
> Foo.class.getMethod("bar").getName() == Foo.class.getMethod("bar").getName()
> yields false.
> 
> Perhaps clazz.equals(conflict) would be better than == in this case?

Mark's fix in r1633810 used conflict.equals(clazz), so it is OK.


The JVM specification (Java 8 edition) in chapter 5.3 Creation and Loading
and Java Language Specification (Java 8 edition) in chapter 12.2 Loading of Classes and Interfaces both contain the same phrase:

"Given the same name, a good class loader should always return the same Class object."