Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Thanks for the review Joe, I added a space and pushed.
cheers
/Joel
> On 22 aug 2015, at 01:12, Joseph D. Darcy wrote:
>
> Belatedly, the
>
>http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/
>
> webrev looks fine other than a missing space in a line of
> AnnotationSuport.java:
>
>208 }catch (Throwable t) { // from
> InvocationHandler::invoke
>
> Thanks,
>
> -Joe
>
> On 3/28/2015 3:24 AM, Joel Borggrén-Franck wrote:
>> Hi,
>>
>>> On 27 Feb 2015, at 11:06, Peter Levart wrote:
>>> On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote:
> On 26 feb 2015, at 22:35, Peter Levart
> wrote:
> On 02/26/2015 10:27 PM, Peter Levart wrote:
>
>> The m.setAccessible(true) for the methods is needed to access methods of
>> non-public annotations, right? This call could be moved to
>> AnnotationType constructor as there it will be performed only once per
>> Method object.
>>
> ...which will have the added benefit in that it will guarantee that only
> one MethodAccessor object per Method will ever be constructed instead of
> two…
>
>
I don’t see this. setAccessible sets override in AccessibleObject, I don’t
see a new MethodAccessor being generated here.
>>> My fault! I was mistakenly thinking of Field objects in the context of
>>> setAccessible(boolean). If you use a Field object in two modes it will
>>> create and retain two different FieldAccessors (because they are different
>>> implementations in case field is final).
>>>
But I agree with you, and setting it as accessible in the AnnotationType
constructor should arguably be more secure since then we know it isn’t
shared since we just got our copy fresh from getDeclaredMethods().
>>> That's a good point from maintainability perspective, yes, if someone some
>>> time decides to "optimize" the AnnotationType. I think it would be nice if
>>> AnnotationType documents that members() method returns Method objects that
>>> are pre-conditioned with setAccessible(true) and that users should not
>>> change this flag.
>>>
>> I don’t want to do this in AnnotationType without a bigger overhaul that may
>> be slightly incompatible and therefor should be 9 only. I do want to
>> backport this fix to 8 however, so here is an alternative solution that
>> should be safe and correct at the cost of extra allocation in the path for
>> custom implementations of annotations (that should be rare).
>>
>> New webrev:
>>
>> http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/
>>
>> cheers
>> /Joel
>
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Belatedly, the
http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/
webrev looks fine other than a missing space in a line of
AnnotationSuport.java:
208 }catch (Throwable t) { // from
InvocationHandler::invoke
Thanks,
-Joe
On 3/28/2015 3:24 AM, Joel Borggrén-Franck wrote:
Hi,
On 27 Feb 2015, at 11:06, Peter Levart wrote:
On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote:
On 26 feb 2015, at 22:35, Peter Levart
wrote:
On 02/26/2015 10:27 PM, Peter Levart wrote:
The m.setAccessible(true) for the methods is needed to access methods of
non-public annotations, right? This call could be moved to AnnotationType
constructor as there it will be performed only once per Method object.
...which will have the added benefit in that it will guarantee that only one
MethodAccessor object per Method will ever be constructed instead of two…
I don’t see this. setAccessible sets override in AccessibleObject, I don’t see
a new MethodAccessor being generated here.
My fault! I was mistakenly thinking of Field objects in the context of
setAccessible(boolean). If you use a Field object in two modes it will create
and retain two different FieldAccessors (because they are different
implementations in case field is final).
But I agree with you, and setting it as accessible in the AnnotationType
constructor should arguably be more secure since then we know it isn’t shared
since we just got our copy fresh from getDeclaredMethods().
That's a good point from maintainability perspective, yes, if someone some time decides
to "optimize" the AnnotationType. I think it would be nice if AnnotationType
documents that members() method returns Method objects that are pre-conditioned with
setAccessible(true) and that users should not change this flag.
I don’t want to do this in AnnotationType without a bigger overhaul that may be
slightly incompatible and therefor should be 9 only. I do want to backport this
fix to 8 however, so here is an alternative solution that should be safe and
correct at the cost of extra allocation in the path for custom implementations
of annotations (that should be rare).
New webrev:
http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/
cheers
/Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Mandy, Paul, what do you think? cheers /Joel > On 28 Mar 2015, at 11:24, Joel Borggrén-Franck wrote: > > Hi, > >> On 27 Feb 2015, at 11:06, Peter Levart wrote: >> On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: > The m.setAccessible(true) for the methods is needed to access methods of > non-public annotations, right? This call could be moved to AnnotationType > constructor as there it will be performed only once per Method object. > ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… >>> I don’t see this. setAccessible sets override in AccessibleObject, I don’t >>> see a new MethodAccessor being generated here. >> >> My fault! I was mistakenly thinking of Field objects in the context of >> setAccessible(boolean). If you use a Field object in two modes it will >> create and retain two different FieldAccessors (because they are different >> implementations in case field is final). >> >>> But I agree with you, and setting it as accessible in the AnnotationType >>> constructor should arguably be more secure since then we know it isn’t >>> shared since we just got our copy fresh from getDeclaredMethods(). >> >> That's a good point from maintainability perspective, yes, if someone some >> time decides to "optimize" the AnnotationType. I think it would be nice if >> AnnotationType documents that members() method returns Method objects that >> are pre-conditioned with setAccessible(true) and that users should not >> change this flag. >> > > I don’t want to do this in AnnotationType without a bigger overhaul that may > be slightly incompatible and therefor should be 9 only. I do want to backport > this fix to 8 however, so here is an alternative solution that should be safe > and correct at the cost of extra allocation in the path for custom > implementations of annotations (that should be rare). > > New webrev: > > http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ > > cheers > /Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
> On 28 Mar 2015, at 13:42, Peter Levart wrote: > On 03/28/2015 11:24 AM, Joel Borggrén-Franck wrote: >> Hi, >> >> >>> On 27 Feb 2015, at 11:06, Peter Levart >>> wrote: >>> On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: >>> > On 26 feb 2015, at 22:35, Peter Levart > > wrote: > On 02/26/2015 10:27 PM, Peter Levart wrote: > > >> The m.setAccessible(true) for the methods is needed to access methods of >> non-public annotations, right? This call could be moved to >> AnnotationType constructor as there it will be performed only once per >> Method object. >> >> > ...which will have the added benefit in that it will guarantee that only > one MethodAccessor object per Method will ever be constructed instead of > two… > > > I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. >>> My fault! I was mistakenly thinking of Field objects in the context of >>> setAccessible(boolean). If you use a Field object in two modes it will >>> create and retain two different FieldAccessors (because they are different >>> implementations in case field is final). >>> >>> But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). >>> That's a good point from maintainability perspective, yes, if someone some >>> time decides to "optimize" the AnnotationType. I think it would be nice if >>> AnnotationType documents that members() method returns Method objects that >>> are pre-conditioned with setAccessible(true) and that users should not >>> change this flag. >>> >>> >> I don’t want to do this in AnnotationType without a bigger overhaul that may >> be slightly incompatible and therefor should be 9 only. I do want to >> backport this fix to 8 however, so here is an alternative solution that >> should be safe and correct at the cost of extra allocation in the path for >> custom implementations of annotations (that should be rare). >> >> New webrev: >> >> >> http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ >> >> >> cheers >> /Joel >> > > Hi Joel, > > If you really must add new methods to Method, ReflectAccess, > LangReflectAccess and ReflectionFactory then you could expose a method called > "invokeUnchecked" that would skip access checks, so you don't have to copy > the Method object just to get a one-time access to the possibly inaccessible > method. IMO this would require quite a lot of investment validating all the possible ways it may be a security concern. I don’t think this is worth it given the very limited use, tough I find the idea interesting. > But this is good as it is, since the code path is normally not used - it's > there only for consistency's sake, right? > Yes, that is my belief. I don’t know of any AnnotatedElements needing this except in testing, of course that isn’t to say there are none. cheers /Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 03/28/2015 11:24 AM, Joel Borggrén-Franck wrote: Hi, On 27 Feb 2015, at 11:06, Peter Levart wrote: On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to "optimize" the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel Hi Joel, If you really must add new methods to Method, ReflectAccess, LangReflectAccess and ReflectionFactory then you could expose a method called "invokeUnchecked" that would skip access checks, so you don't have to copy the Method object just to get a one-time access to the possibly inaccessible method. But this is good as it is, since the code path is normally not used - it's there only for consistency's sake, right? Regards, Peter
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi, > On 27 Feb 2015, at 11:06, Peter Levart wrote: > On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: >>> On 26 feb 2015, at 22:35, Peter Levart >>> wrote: >>> On 02/26/2015 10:27 PM, Peter Levart wrote: >>> The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. >>> ...which will have the added benefit in that it will guarantee that only >>> one MethodAccessor object per Method will ever be constructed instead of >>> two… >>> >>> >> I don’t see this. setAccessible sets override in AccessibleObject, I don’t >> see a new MethodAccessor being generated here. > > My fault! I was mistakenly thinking of Field objects in the context of > setAccessible(boolean). If you use a Field object in two modes it will create > and retain two different FieldAccessors (because they are different > implementations in case field is final). > >> But I agree with you, and setting it as accessible in the AnnotationType >> constructor should arguably be more secure since then we know it isn’t >> shared since we just got our copy fresh from getDeclaredMethods(). > > That's a good point from maintainability perspective, yes, if someone some > time decides to "optimize" the AnnotationType. I think it would be nice if > AnnotationType documents that members() method returns Method objects that > are pre-conditioned with setAccessible(true) and that users should not change > this flag. > I don’t want to do this in AnnotationType without a bigger overhaul that may be slightly incompatible and therefor should be 9 only. I do want to backport this fix to 8 however, so here is an alternative solution that should be safe and correct at the cost of extra allocation in the path for custom implementations of annotations (that should be rare). New webrev: http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ cheers /Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: On 26 feb 2015, at 22:35, Peter Levart wrote: On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two… I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. My fault! I was mistakenly thinking of Field objects in the context of setAccessible(boolean). If you use a Field object in two modes it will create and retain two different FieldAccessors (because they are different implementations in case field is final). But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). That's a good point from maintainability perspective, yes, if someone some time decides to "optimize" the AnnotationType. I think it would be nice if AnnotationType documents that members() method returns Method objects that are pre-conditioned with setAccessible(true) and that users should not change this flag. Regards, Peter cheers /Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi, > On 27 feb 2015, at 01:04, Mandy Chung 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
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
> On 26 feb 2015, at 22:35, Peter Levart wrote: > On 02/26/2015 10:27 PM, Peter Levart wrote: >> The m.setAccessible(true) for the methods is needed to access methods of >> non-public annotations, right? This call could be moved to AnnotationType >> constructor as there it will be performed only once per Method object. > > ...which will have the added benefit in that it will guarantee that only one > MethodAccessor object per Method will ever be constructed instead of two… > I don’t see this. setAccessible sets override in AccessibleObject, I don’t see a new MethodAccessor being generated here. But I agree with you, and setting it as accessible in the AnnotationType constructor should arguably be more secure since then we know it isn’t shared since we just got our copy fresh from getDeclaredMethods(). cheers /Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 2/26/2015 5:01 PM, Peter Levart wrote: On 02/27/2015 01:07 AM, Mandy Chung wrote: Thanks for the test. The question is what the spec says about SecurityException, or it should require the value() method be public or there is a reason to support a non-public value() method? The value() method is always public (since it's an interface method), but the interface need not be public. Thanks for the clarification. So I don't think we should prevent access to repeatable annotation instances just because the container annotation type of the repeatable annotation is not public. The call to setAccessible(true) should be wrapped by doPrivileged and should be performed in AnnotationType constructor and not sprinkled in other places that need to invoke the Method(s). This is by no means less secure as it doesn't matter what part of code makes the Method object setAccessible(true) if it is a shared Method object. Will wait for Joel to say more about this. I agree with your observation. Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 02/27/2015 01:07 AM, Mandy Chung wrote: Thanks for the test. The question is what the spec says about SecurityException, or it should require the value() method be public or there is a reason to support a non-public value() method? The value() method is always public (since it's an interface method), but the interface need not be public. Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 02/27/2015 01:07 AM, Mandy Chung wrote:
On 2/26/15 2:04 PM, Peter Levart wrote:
I verified this with the following code:
:
... it works without problems and prints the expected:
Thanks for the test. The question is what the spec says about
SecurityException, or it should require the value() method be public
or there is a reason to support a non-public value() method?
Mandy
Well, currently, with pre-JDK8 APIs, one has access to annotation
instances of types that are otherwise inaccessible. For example:
public class Test {
public static void main(String[] args) throws Exception {
//
TestSibling.class.getDeclaredAnnotation(TestSibling.PrivateAnn.class);
// >>> javac Error: The type TestSibling.PrivateAnn is not visible
// but:
Annotation privateAnn =
TestSibling.class.getDeclaredAnnotations()[0];
System.out.println(privateAnn);
// >>> @TestSibling$PrivateAnn()
}
}
@TestSibling.PrivateAnn()
class TestSibling {
@Retention(RetentionPolicy.RUNTIME)
private @interface PrivateAnn {
}
}
So I don't think we should prevent access to repeatable annotation
instances just because the container annotation type of the repeatable
annotation is not public.
The call to setAccessible(true) should be wrapped by doPrivileged and
should be performed in AnnotationType constructor and not sprinkled in
other places that need to invoke the Method(s). This is by no means less
secure as it doesn't matter what part of code makes the Method object
setAccessible(true) if it is a shared Method object.
Regards, Peter
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 2/26/15 2:04 PM, Peter Levart wrote: I verified this with the following code: : ... it works without problems and prints the expected: Thanks for the test. The question is what the spec says about SecurityException, or it should require the value() method be public or there is a reason to support a non-public value() method? Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
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. The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? I think so. 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. Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 02/26/2015 10: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 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 verified this with the following code:
import java.lang.annotation.Annotation;
import java.lang.annotation.Repeatable;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.reflect.AnnotatedElement;
import java.util.Arrays;
@CustomAnnTest.Containee("a")
@CustomAnnTest.Containee("b")
@CustomAnnTest.Containee("c")
public class CustomAnnTest {
@Retention(RetentionPolicy.RUNTIME)
@Repeatable(Container.class)
public @interface Containee {
String value();
}
@Retention(RetentionPolicy.RUNTIME)
public @interface Container {
Containee[] value();
}
public static class MyElement implements AnnotatedElement {
@Override
public T getAnnotation(Class
annotationClass) {
for (Annotation ann : getDeclaredAnnotations()) {
if (ann.annotationType() == annotationClass) {
return annotationClass.cast(ann);
}
}
return null;
}
@Override
public Annotation[] getAnnotations() {
return getDeclaredAnnotations();
}
@Override
public Annotation[] getDeclaredAnnotations() {
return new Annotation[] {
new MyContainer(
new MyContainee("A"),
new MyContainee("B"),
new MyContainee("C")
)
};
}
static class MyContainee implements Containee {
final String value;
MyContainee(String value) {
this.value = value;
}
@Override
public String value() {
return value;
}
@Override
public Class annotationType() {
return Containee.class;
}
@Override
public String toString() {
return "@" + annotationType().getName() + "(value=" +
value + ")";
}
}
static class MyContainer implements Container {
final Containee[] value;
MyContainer(Containee ... value) {
this.value = value;
}
@Override
public Containee[] value() {
return value;
}
@Override
public Class annotationType() {
return Container.class;
}
@Override
public String toString() {
return "@" + annotationType().getName() + "(value=" +
Arrays.toString(value) + ")";
}
}
}
static void test(AnnotatedElement ae) {
System.out.println(ae + ":");
System.out.println(Arrays.toString(ae.getDeclaredAnnotations()));
System.out.println(ae.getAnnotation(Container.class));
System.out.println(ae.getAnnotation(Containee.class));
System.out.println(Arrays.toString(ae.getDeclaredAnnotationsByType(Containee.class)));
System.out.println();
}
public static void main(String[] args) {
test(CustomAnnTest.class);
test(new MyElement());
}
}
... it works without problems and prints the expected:
class jdk.test.CustomAnnTest:
[@jdk.test.CustomAnnTest$Container(value=[@jdk.test.CustomAnnTest$Containee(value=a),
@jdk.test.CustomA
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 02/26/2015 10:27 PM, Peter Levart wrote: The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. ...which will have the added benefit in that it will guarantee that only one MethodAccessor object per Method will ever be constructed instead of two... Peter Regards, Peter
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
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 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. The m.setAccessible(true) for the methods is needed to access methods of non-public annotations, right? This call could be moved to AnnotationType constructor as there it will be performed only once per Method object. Regards, Peter
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On 2/26/2015 12:30 AM, Joel Borggrén-Franck wrote: I realize now that this code will probably never have worked on other hypothetical implementations of Annotations. This is what I was wondering when I see it expect sun.reflect.annotation.AnnotationType. 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. I think throwing here might be the best option, especially since we will probably already have failed in the AnnotationType.getInstance check. I haven't studied closely the specification about support for alternate implementation. One thing I would say is that if the implementation never works for alternate implementation, to fix JDK-8073056, I would simply remove line 207-218. Perhaps file a new issue to follow up the support for alternate implementation if appropriate. Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On Feb 26, 2015, at 9:30 AM, Joel Borggrén-Franck
wrote:
>> 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.
>
From what you say it seems like throwing a USO might be the best option with an
"unsupported annotation container" message. Although i presume being a Proxy is
not entirely sufficient to identify that the container comes from an external
implementation?
Paul.
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi Mandy,
> On 25 feb 2015, at 23:19, Mandy Chung 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
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
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
204 }catch (Throwable t) { // from InvocationHandler::invoke
Missing space between } and catch
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.
Mandy
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi,
> On 25 feb 2015, at 13:16, Paul Sandoz wrote:
> On Feb 25, 2015, at 12:59 PM, Joel Borggrén-Franck
> wrote:
>
>> Hi Paul,
>>
>> Yes that would indeed be possible, but after some internal discussions we
>> though it safer to reuse the Proxy invocation path.
>>
>
> Ok, i claim ignorance as to why that is so :-)
>
> Is there any performance impact since all code will go through the proxy? It
> might be possible to check if the security manager is enabled before going
> through that path?
Arguably this should actually be faster. For all “our” annotations the Proxy
invocation handler will be the final invoker anyway, so this is just cutting
out the middle.
> If not I think you should add a comment on the effectively dead code path of
> direct method invocation.
>
Agreed.
>
> 213 } catch (IllegalAccessException| // couldn't loosen security
> 214 IllegalArgumentException | // parameters doesn't match
> 215 InvocationTargetException | // the value method threw an
> exception
> 216 ClassCastException e) {
>
> 217 throw invalidContainerException(container, e);
> 218 } catch (Throwable t) { // from InvocationHandler::invoke
> 219 throw invalidContainerException(container, t);
> 220 }
> 221 }
> 222
>
> You could compress all this down to just catching Throwable. It seems quite a
> large net that may inadvertently catch other runtime exceptions.
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/
cheers
/Joel
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
On Feb 25, 2015, at 12:59 PM, Joel Borggrén-Franck
wrote:
> Hi Paul,
>
> Yes that would indeed be possible, but after some internal discussions we
> though it safer to reuse the Proxy invocation path.
>
Ok, i claim ignorance as to why that is so :-)
Is there any performance impact since all code will go through the proxy? It
might be possible to check if the security manager is enabled before going
through that path? If not I think you should add a comment on the effectively
dead code path of direct method invocation.
213 } catch (IllegalAccessException| // couldn't loosen security
214 IllegalArgumentException | // parameters doesn't match
215 InvocationTargetException | // the value method threw an
exception
216 ClassCastException e) {
217 throw invalidContainerException(container, e);
218 } catch (Throwable t) { // from InvocationHandler::invoke
219 throw invalidContainerException(container, t);
220 }
221 }
222
You could compress all this down to just catching Throwable. It seems quite a
large net that may inadvertently catch other runtime exceptions.
Paul.
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi Paul, Yes that would indeed be possible, but after some internal discussions we though it safer to reuse the Proxy invocation path. cheers /Joel > On 25 feb 2015, at 12:55, Paul Sandoz wrote: > > Hi Joel, > > I could be missing something here but would it be possible to wrap the call > to m.setAccessible in a doPriv block instead? > > Paul. > > On Feb 24, 2015, at 11:49 AM, Joel Borggrén-Franck > wrote: > >> Hi, >> >> Here is a fix for an issue with repeating annotations when a security >> manager is set. >> >> Fix is to use the Proxy invocation handler to unwrap the array containing >> the repeating annotations. In theory it might be possible to have instances >> of Annotations that are not implemented using Proxies, so the old code which >> is independent of implementation is kept as a fall-back, but the user or >> these theoretical annotations will have to configure the security policy >> accordingly. >> >> http://cr.openjdk.java.net/~jfranck/8073056/webrev.00/ >> https://bugs.openjdk.java.net/browse/JDK-8073056 >> >> cheers >> /Joel >
Re: RFR 8073056: Repeating annotations throws java.security.AccessControlException with a SecurityManager
Hi Joel, I could be missing something here but would it be possible to wrap the call to m.setAccessible in a doPriv block instead? Paul. On Feb 24, 2015, at 11:49 AM, Joel Borggrén-Franck wrote: > Hi, > > Here is a fix for an issue with repeating annotations when a security manager > is set. > > Fix is to use the Proxy invocation handler to unwrap the array containing the > repeating annotations. In theory it might be possible to have instances of > Annotations that are not implemented using Proxies, so the old code which is > independent of implementation is kept as a fall-back, but the user or these > theoretical annotations will have to configure the security policy > accordingly. > > http://cr.openjdk.java.net/~jfranck/8073056/webrev.00/ > https://bugs.openjdk.java.net/browse/JDK-8073056 > > cheers > /Joel
