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

[Joel Borggrén-Franck]

Hi,

> On 27 feb 2015, at 01:04, Mandy Chung <mandy.chung at 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