Hi, > On 27 feb 2015, at 01:04, Mandy Chung <mandy.ch...@oracle.com> wrote: > > On 2/26/15 1:27 PM, Peter Levart wrote: >> >> On 02/26/2015 05:34 PM, Mandy Chung wrote: >>>> The problem is with the default method >>>> AnnotatedElement::getDeclaredAnnotationsByType. If someone has an external >>>> implementation of AnnotatedElement (and one of the lessons from adding >>>> these methods in 8 was that there are other implementations) then if that >>>> external implementation haven’t added getDeclaredAnnotationsByType they >>>> will call into our AnnotationSupport. This is all fine if that external >>>> representation of AnnotatedElement uses our representation of Annotations, >>>> with Proxies and all. But I suspect that the conditions that would end up >>>> taking the non-proxy code path in the patch, will already fail in the >>>> check: >>>> >>>> AnnotationType annoType = >>>> AnnotationType.getInstance(containerClass); >>>> if (annoType == null) >>>> throw invalidContainerException(container, null); >>>> >>>> So what is the best thing to do here if the annotation is not Proxy based >>>> and is coming from an implementation of the AnnotatedElement interface >>>> that we don’t control and that is missing the methods added in 8? >>> >>> I did a quick search on my corpus and find only one reference to >>> sun.reflect.annotation.AnnotationType. Looks like external implementation >>> doesn't get pass this. >> >> Now I'm missing something. Why would a custom non-Proxy based annotation not >> pass the above code? > > I had assumed that AnnotationType.getInstance also expects the implementation > of the annotation type is sun.reflect.annotation.AnnotationType. I don't > know enough about this area and that's just my observation. Hope Joel will > say more details. >> I don't see anything in AnnotationType implementation that would be >> dependent on annotation implementation to be a Proxy. AnnotationType only >> deals with the annotation type, which is an interface, not with >> implementation.
I realized this on my way home, and Peter is right here. There is nothing special for “our” annotations in AnnotationType::getInstance. >> >> The m.setAccessible(true) for the methods is needed to access methods of >> non-public annotations, right? > > I think so. Yes, the method on the interface will always (pre 9 at least) be public, the interface might not be accessible. I have toyed with the idea of fetching the method form the impl instead, but that has the same issues, and is arguably worse from a security perspective. >> This call could be moved to AnnotationType constructor as there it will be >> performed only once per Method object. >> > If the specification supports other implementation, it seems to me that > calling setAccessible(true) should be wrapped with doPrivileged unless the > specification states the "suppressAccessCheck" permission is required; > otherwise, SecurityException will be thrown. It'd be good to clarify what > that code is intended for. > There is nothing in the spec about any security exceptions here. But on the other hand, there is nothing in the spec for AnnotatedElement::getAnnotationsByType specifying throwing anything that a custom implementation of AnnotatedElement using the default methods may throw. But I’ll take this back to the drawing board, there is some things I want to explore. However performance is at best a very distant third priority here, after security and compatibility. cheers /Joel
