Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metadata reading should never use ASM for java.* and javax.* types (in particular on JDK 8) [SPR-11719] #16341

Closed
spring-issuemaster opened this issue Apr 22, 2014 · 6 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Apr 22, 2014

Oliver Drotbohm opened SPR-11719 and commented

If running JDK 8 and the ClassPathScanningCandidateComponentProvider encounters an annotation type during scanning (e.g. in Spring Data JPA where we explicitly scan for interfaces) it tries to read meta-data from java.lang.annotation.Annotation. This type of course was compiled with -target 1.8 for JDK 8 and thus the ASM reader will choke on it. Even if ASM didn't, we shouldn't actually process JDK classes with the ASM reader as it can cause issues with the Security Manager.


Affects: 3.2.8, 4.0.3

Issue Links:

  • #16279 Basic Java 8 bytecode compatibility for Spring 3.2.x through ASM 5.0.2
  • #12984 Allow AnnotationTypeFilter to consider interfaces as well
  • #16340 Fix/optimize handling of @Bean method override regression with return type narrowing on JDK 8
  • #21208 Reduce ClassUtils.forName overhead (in particular for annotation introspection purposes)

Referenced from: commits 5ab7076, 945335d, e379e77, 9c45755, 2c1203d

Backported to: 3.2.9

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 22, 2014

Juergen Hoeller commented

It turns out that we had code for resolving "java." types via a ClassLoader before... but only in AssignableTypeFilter. In AnnotationTypeFilter, we only had it for superclass resolution since we never considered interfaces there originally. Then, in #12984, somebody came along and added support for a considerInterfaces flag... but didn't add the corresponding interface resolution code along the lines of what we had for superclasses there :-)

Juergen

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 22, 2014

Juergen Hoeller commented

Fixed through resolution code similar to the one in AssignableTypeFilter.

While being at it, I've also fixed AbstractTypeHierarchyTraversingFilter's considerInterfaces treatment to work without considerInherited as well.

Juergen

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 26, 2014

Itay Koren commented

I'm experiencing a problem that seems related - While using an app compiled with -target 1.7, running on JDK 8 and spring 3.2.8 , the ASM reader explodes when reading JDK related classes.
However, the classes involved belong to "javax" package, and not "java" (for example, javax.sql.ConnectionEventListener and javax.xml.bind.annotation.adapters.XmlAdapter).
In addition, the filter involved is the AssignableTypeFilter, not the AnnotationTypeFilter.

Should I open a new bug?

Partial stack trace:

org.springframework.core.NestedIOException: ASM ClassReader failed to parse class file - probably due to a new Java cl
ass file version that isn't supported yet: class path resource [javax/sql/ConnectionEventListener.class]; nested excep
tion is java.lang.IllegalArgumentException
at org.springframework.core.type.classreading.SimpleMetadataReader.<init>(SimpleMetadataReader.java:56)
at org.springframework.core.type.classreading.SimpleMetadataReaderFactory.getMetadataReader(SimpleMetadataRead
erFactory.java:80)
at org.springframework.core.type.classreading.CachingMetadataReaderFactory.getMetadataReader(CachingMetadataRe
aderFactory.java:102)
at org.springframework.core.type.classreading.SimpleMetadataReaderFactory.getMetadataReader(SimpleMetadataRead
erFactory.java:76)
at org.springframework.core.type.filter.AbstractTypeHierarchyTraversingFilter.match(AbstractTypeHierarchyTrave
rsingFilter.java:105)
at org.springframework.core.type.filter.AbstractTypeHierarchyTraversingFilter.match(AbstractTypeHierarchyTrave
rsingFilter.java:95)
at org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.isCandidateComponent(Cla
ssPathScanningCandidateComponentProvider.java:333)
at org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.findCandidateComponents(
ClassPathScanningCandidateComponentProvider.java:267)
at org.springframework.context.annotation.ClassPathBeanDefinitionScanner.doScan(ClassPathBeanDefinitionScanner
.java:242)
at org.springframework.context.annotation.ClassPathBeanDefinitionScanner.scan(ClassPathBeanDefinitionScanner.j
ava:220)

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 27, 2014

Juergen Hoeller commented

No need to open a new issue; let's deal with it as part of this one.

It's a good point that the javax.* classes in the JDK's rt.jar suffer from the same problem. Fortunately, there aren't many of those appearing as interfaces or superclasses in typical Spring beans - however, there are still a few cases, as you outlined, in particular from JDK-packaged EE APIs.

The good news: As of Spring Framework 3.2.9, we'll ship a repackaged variant of ASM 5, which is able to parse class files from JDK 8's rt.jar. This is available in the latest Spring Framework 3.2.9 snapshots already.

The bad news: Trying to read class files from rt.jar is a general issue, since it might lead to security exceptions in a JVM security manager environment. We'd at least need to be defensive there, catching such exceptions and falling back to reflective loading. Ideally, we'd be able to find out about a class to be loaded from rt.jar and fall back immediately, without letting a security exception happen. I'll see what we can do there.

Juergen

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 27, 2014

Juergen Hoeller commented

In fact, it doesn't hurt to load all javax.* types reflectively if possible, even outside of rt.jar, since those types are rather unlikely to be susceptible to application-level initialization or weaving problems. After all, they are usually loaded early by the JDK anyway or are included in an EE server's common lib directory, and loaded before the application's startup.

That said, we are defensively catching any exceptions and errors from loadClass attempts in the filters though, to also deal with transitive NoClassDefFoundErrors, just in case an incomplete EE API happens to be on the classpath. This should be a fine compromise overall and avoid unnecessary parsing of class files for commonly loaded API classes.

Juergen

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Apr 27, 2014

Itay Koren commented

First of all, thanks a lot for the quick and thorough response!

I saw that indeed the ASM version used in 3.2.9 should no longer explode when loading such classes, but I wanted to point out the problem, especially since as you and Oliver both mentioned, there are possible security exceptions if it's not handled.

Your efforts are truly appreciated.

Itay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.