8339192: Native annotation parsing code of deprecated annotations causes crash

[dholmes-ora]

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

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8339192: Native annotation parsing code of deprecated annotations causes crash (Bug - P4)

Reviewers

• John R Rose (@rose00 - Reviewer)
• Markus Grönlund (@mgronlun - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21030/head:pull/21030
$ git checkout pull/21030

Update a local copy of the PR:
$ git checkout pull/21030
$ git pull https://git.openjdk.org/jdk.git pull/21030/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21030

View PR using the GUI difftool:
$ git pr show -t 21030

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21030.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8339192: Native annotation parsing code of deprecated annotations causes crash

Reviewed-by: jrose, mgronlun

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 158 new commits pushed to the master branch:

• fdc16a3: 8340480: Bad copyright notices in changes from JDK-8339902
• 296b496: 8340353: Remove CompressedOops::ptrs_base
• fde8508: 8339902: Open source couple TextField related tests
• 75d5e11: 8340400: Shenandoah: Whitebox breakpoint GC requests may cause assertions
• 15ae115: 8340166: [REDO] CDS: Trim down minimum GC region alignment
• ec3cba0: 8340399: Update comment in SourceVersion for language evolution history
• d1d8240: 8340308: PassFailJFrame: Make rows default to number of lines in instructions
• bc36ace: 8340271: Open source several AWT Robot tests
• 5f3e7aa: 8339735: Remove references to Applet in core-libs/security APIs
• 2ada313: 8340329: (fs) Message of NotLinkException thrown by FIles.readSymbolicLink does not include file name (win)
• ... and 148 more: https://git.openjdk.org/jdk/compare/4ff72dc57e65e99b129f0ba28196994edf402018...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@dholmes-ora The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (1e7f7b00)
• 00: Full (c71a7730)

[dholmes-ora]

Thanks for looking at this @liach !

[liach]

Indeed, the specs only require us to correctly recognize completely correct format and silently ignore anything that's wrong. For a partially correct structure (including structurally valid but semantically invalid, such as duplicate members), any leniency should be fine.

[rose00]

Reviewed. Nice tests. Thanks for fixing the problem in both places.

I do think, if we have to do more of this stuff in the future, that maybe we should be writing special accessors for the CP itself, ones which can return an error indication instead of throwing an assert or crashing the VM.

[dholmes-ora]

Thanks for taking a look @rose00 ! Can you click the approve button please. :)

[dholmes-ora]

Thank you for the review @rose00 and @mgronlun !

/integrate

[openjdk]

Going to push as commit 969c2af.
Since your change was applied there have been 158 commits pushed to the master branch:

• fdc16a3: 8340480: Bad copyright notices in changes from JDK-8339902
• 296b496: 8340353: Remove CompressedOops::ptrs_base
• fde8508: 8339902: Open source couple TextField related tests
• 75d5e11: 8340400: Shenandoah: Whitebox breakpoint GC requests may cause assertions
• 15ae115: 8340166: [REDO] CDS: Trim down minimum GC region alignment
• ec3cba0: 8340399: Update comment in SourceVersion for language evolution history
• d1d8240: 8340308: PassFailJFrame: Make rows default to number of lines in instructions
• bc36ace: 8340271: Open source several AWT Robot tests
• 5f3e7aa: 8339735: Remove references to Applet in core-libs/security APIs
• 2ada313: 8340329: (fs) Message of NotLinkException thrown by FIles.readSymbolicLink does not include file name (win)
• ... and 148 more: https://git.openjdk.org/jdk/compare/4ff72dc57e65e99b129f0ba28196994edf402018...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora Pushed as commit 969c2af.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.