7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Alan Bateman]

On 24/06/2013 19:23, Peter Levart wrote:
>
> Hi Alan,
>
> I have prepared the 2nd revision of the patch:
>
> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.02/
>
> There is a little change in AnnotationParser's alternative parsing 
> method. This time the parser does not assume anything about 
> annotations being parsed (previously it assumed that they had RUNTIME 
> retention). The only difference from standard parsing is that only the 
> select annotation types are parsed and the rest are quickly skipped. 
> Infinite recursion is broken by the special cased evaluation in 
> AnnotationType constructor:
>
>   129         // Initialize retention,&  inherited fields.  Special treatment
>   130         // of the corresponding annotation types breaks infinite recursion.
>   131         if (annotationClass != Retention.class&&
>   132             annotationClass != Inherited.class) {
>   133             JavaLangAccess jla = sun.misc.SharedSecrets.getJavaLangAccess();
>   134             Map<Class<? extends Annotation>, Annotation>  metaAnnotations =
>   135                 AnnotationParser.parseAnnotations(
>   136                     jla.getRawClassAnnotations(annotationClass),
>   137                     jla.getConstantPool(annotationClass),
>   138                     annotationClass,
>   139                     Retention.class, Inherited.class
>   140                 );
>   141             Retention ret = (Retention) metaAnnotations.get(Retention.class);
>   142             retention = (ret == null ? RetentionPolicy.CLASS : ret.value());
>   143             inherited = metaAnnotations.containsKey(Inherited.class);
>   144         }
>   145         else {
>   146             retention = RetentionPolicy.RUNTIME;
>   147             inherited = false;
>   148         }
>
> This is enough to break recursion. The RUNTIME and !inherited 
> assumptions for @Retention and @Inherited meta-annotations are 
> therefore localized in this code only.
>
> I have also added two tests. The one for detecting deadlock situation 
> and the other for consistent parsing of mutually recursive annotations 
> in presence of separate compilation.

Sorry for the delay getting back to you on this, I've been busy with 
other things.

I'm not an expert on the annotation code but the updated approach to 
break the recursion looks good good to me (and a clever approach). Joel 
has been on vacation but he told me that he plans to look at this too 
(I'm happy to sponsor it in the mean-time as I think the approach is 
good and we should get this fix in).

There's another usage of AnnotationParser.parseAnnotation in 
TypeAnnotationParser that will need to be updated (I only noticed it 
when I grabbed your patch to try it).

A minor comment is that the alignment of the parameter declarations when 
they go over multiple lines should probably be fixed up to be consistent 
with the other usages.

Thanks for adding tests. One comment on AnnotationTypeDeadlockTest is 
that the join(500) might not be sufficient on a very busy system (say 
when running tests concurrently) or on a low-end embedded system. So I 
think it would be good to bump this up x10. An alternative would be to 
not set the daemon status and just let the test timeout if there is a 
deadlock. The spin-waits can consume cycles when running tests 
concurrently but I don't expect it's an issue here.

A typo in the @summary of AnnotationTypeRuntimeAssumptionTest ("si" -> 
"is"). I guess I'd go for slightly shorter lines to make future 
side-by-side reviews easier.

Otherwise, this looks good to me.

-Alan.