> On 28 Mar 2015, at 13:42, Peter Levart <peter.lev...@gmail.com> wrote: > On 03/28/2015 11:24 AM, Joel Borggrén-Franck wrote: >> Hi, >> >> >>> On 27 Feb 2015, at 11:06, Peter Levart <peter.lev...@gmail.com> >>> wrote: >>> On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: >>> >>>>> On 26 feb 2015, at 22:35, Peter Levart <peter.lev...@gmail.com> >>>>> >>>>> 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
