RFR: 8364655: Loading class with nested annotations causes stack overflow in VM [v3]

[Johan Sjölen]

> Hi,
> 
> `skip_annotation` and `skip_annotation_value` are two mutually recursive functions calling each other in order to skip over classfile annotations. If a classfile contains a highly nested annotation, then this will lead to a stack overflow and a subsequent crash of the JVM. I propose that we insert a recursion limit to prevent this from happening. This recursion limit will make the annotation parsing to bail out on the JVM side, skipping the rest of the annotations present. 
> 
> An example of Java code where we end up with nested annotations is this:
> 
> 
> import java.lang.annotation.*;
> 
> @Retention(RetentionPolicy.RUNTIME)
> public @interface Foo { Bar value(); }
> 
> @Retention(RetentionPolicy.RUNTIME)
> @interface Bar { Baz value();}
> 
> @Retention(RetentionPolicy.RUNTIME)
> @interface Baz { BarBaz value(); }
> 
> @Retention(RetentionPolicy.RUNTIME)
> @interface BarBaz { End value(); }
> 
> @Retention(RetentionPolicy.RUNTIME)
> @interface End { int value(); }
> 
> 
> @Foo(value = @Bar(value = @Baz(value = @BarBaz(value = @End(value=1)))))
> class Lol {
> 
>     @Deprecated // <--- This annotation might be missed with my change, as the depth limit is 5
>     void foobar();
> };
> 
> 
> Today, it seems that Java disallows cyclic interface annotations, this is why in order to achieve the nesting in `Lol` we need to define multiple classes (`Foo`, `Bar`, and so on). Attempting to define a `Foo` with a `Foo value()` will fail compilation.
> 
> I think that such a high nesting of annotations is unlikely to occur in normal Java code. The only 'public' consumer of annotations is JFR, who looks for `@Deprecated` annotations. Typically, the JVM parses these annotations is to gain access to a select few JDK-internal annotations, and we trust our own code to construct classfiles without egregious nesting. To quote John Rose in the JBS issue:
> 
>> If a JVM-detected annotation is preceded by a toxic input, it will be masked by a bailout. So be it. JVM-detected annotations are for power users; such users can take measures that their code will not be polluted with toxic annotations. 
> 
> All classfile annotations are also parsed by Java-code, so we're not skipping general annotation parsing.
> 
> A regression test has been added, where we check that the JVM does not crash when provided with a large amount of nesting.
> 
> Thanks!

Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:

  Copyright

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/28674/files
  - new: https://git.openjdk.org/jdk/pull/28674/files/5cceb010..a705bc1f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=28674&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=28674&range=01-02

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/28674.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/28674/head:pull/28674

PR: https://git.openjdk.org/jdk/pull/28674