RFR: 8339192: Native annotation parsing code of deprecated annotations causes crash

[Chen Liang]

On Tue, 17 Sep 2024 04:37:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

> There are a couple of runtime visible annotations (`@Deprecated`, `@Contended`) that the VM processes during classfile parsing. These are expected to have a form that matches the corresponding Java source code in the JDK. The original test cases for this bug fuzzed the annotation entry in the classfile such that the correspondence was lost and so assertions and other errors could be triggered (depending on debug or release builds). The fix is to fully validate the annotation entries that appear to match, and to ignore them otherwise (which is what is required for unknown attributes/annotations).
> 
> Each of the jcod test cases is targeted at a specific change in the code (typically the removal of an assertion). There is no intent to try and write exhaustive tests for all possible malformities that might exist for an annotation entry. The original submitted test cases now run without error.
> 
> Testing was verified manually by adding debug printing to show we rejected each case as expected. Unfortunately there is no existing mechanism to read back the VM's view of what annotations are present, nor is there any logging to take advantage of. I briefly considered adding a WhiteBox API to query the annotations applied in the VM but I deemed it not worth the effort, particularly because it didn't really help validate the tests in many cases. For example, by default no class/method/field is "Deprecated for Removal" so a test that skips `forRemoval` setting just leaves you with the default situation. You can't actually tell a bad annotation was skipped.
> 
> Testing
>  - new tests as described above 
>  - tier 1-3 sanity
> 
> Thanks

src/hotspot/share/classfile/classFileParser.cpp line 1174:

> 1172:           if (cp->is_within_bounds(boolean_value_index) &&
> 1173:               cp->tag_at(boolean_value_index).is_int() &&
> 1174:               cp->int_at(boolean_value_index) == 1) {

Should we use `== 1` or `(... & 1) == 1` for boolean conversions?

src/hotspot/share/classfile/classFileParser.cpp line 1204:

> 1202:       if (count == 1
> 1203:         && s_size == (index - index0)  // match size
> 1204:         && s_tag_val == *(abase + tag_off)

I see in previous lines that the count restriction on `@Contended` remains, and such a restriction is absent for `@Deprecated`. Intentional?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21030#discussion_r1762317766
PR Review Comment: https://git.openjdk.org/jdk/pull/21030#discussion_r1762330005