RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager

[Joel Borggrén-Franck]

Hi Mandy,

> On 25 feb 2015, at 23:19, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> 
> On 2/25/2015 5:19 AM, Joel Borggrén-Franck wrote:
>> InvocationHandler::invoke unfortunately throws Throwable, but I restructured it a bit so it is easier to follow.
>> 
>> http://cr.openjdk.java.net/~jfranck/8073056/webrev.01/
> 
> 196      InvocationHandler handler = Proxy.getInvocationHandler(container);
> 
> Do you want to ensure it's from our implementation?
> i.e.  sun.reflect.annotation.AnnotationInvocationHandler
> 

I think I have a mail somewhere where Joe states that annotations were designed so that there could be other implementations of the invocation handler.  

> 204      }catch (Throwable t) { // from InvocationHandler::invoke
> 
> Missing space between } and catch

Will fix.

> 182     // According to JLS the container must have an array-valued value
> 183     // method. Get the AnnotationType, get the "value" method and invoke
> 184     // it to get the content.
> 190     Method m = annoType.members().get("value");
> 212     m.setAccessible(true);
> 
> I am missing something here. I read the code and I think
> annoType is of sun.reflect.annotation.AnnotationType type.
> Does the old implementation still exist that is supported?
> Which method is it in the old implementation?  If it's still
> supported, as Class.getAnnotationsByType is not specified to
> require any permission check nor throw any SecurityException,
> and it seems that setAccessible(true) should be wrapped with
> doPrivileged.
> 
> If it's not used in our implementation after your patch,
> perhaps better to take m.setAccessible(true) out.

I realize now that this code will probably never have worked on other hypothetical implementations of Annotations. 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 think throwing here might be the best option, especially since we will probably already have failed in the AnnotationType.getInstance check.

cheers
/Joel