Summary: | ImportHandler.resolveClass() fails to report conflicting import when called repeatedly | ||
---|---|---|---|
Product: | Tomcat 8 | Reporter: | Konstantin Kolinko <knst.kolinko> |
Component: | EL | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | minor | ||
Priority: | P2 | ||
Version: | 8.0.14 | ||
Target Milestone: | ---- | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: | 2014-10-22_tc8_57132_testcases.patch |
Description
Konstantin Kolinko
2014-10-22 21:56:39 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 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). 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... Third issue fixed in 8.0.x for 8.0.15 onwards. (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? (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." |