8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 #8280

smita-kamath · 2022-04-18T05:06:26Z

When input length provided to the intrinsic is 8192, only 7680 bytes are processed as the intrinsic operates on multiples of 768 bytes.
In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method,
dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as PARALLEL_LEN was set to 8192.
Since the intrinsic only processed 7680 bytes, the rest output was incorrect.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

Reviewers

Anthony Scarpino (@ascarpino - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8280/head:pull/8280
$ git checkout pull/8280

Update a local copy of the PR:
$ git checkout pull/8280
$ git pull https://git.openjdk.java.net/jdk pull/8280/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8280

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8280.diff

…bytes

bridgekeeper · 2022-04-18T05:07:32Z

👋 Welcome back svkamath! 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 · 2022-04-18T05:09:11Z

@smita-kamath The following label will be automatically applied to this pull request:

security

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 · 2022-04-18T05:13:13Z

Webrevs

01: Full - Incremental (3c917fab)
00: Full (ca08ccd9)

AlanBateman · 2022-04-18T08:58:46Z

It's good that this issue has been found. There seems to be an intrinsic for aarch64 with a vectorized GCM implementation, I guess is must also work in multiples of 768 bytes, so is this change okay there too?

openjdk · 2022-04-18T15:12:48Z

@smita-kamath 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:

8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

Reviewed-by: ascarpino

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 104 new commits pushed to the master branch:

1e79ded: 8284889: runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java#custom-cl-zgc timed out
414918d: 8285389: EdDSA trimming zeros
293bc5e: 8129778: Few awt test fail for Solaris 11 with RuntimeException
36f2e52: 8225777: java/awt/Mixing/MixingOnDialog.java fails on Ubuntu
32593df: 8279888: Local variable independently used by multiple loops can interfere with loop optimizations
4c22a9b: 8282823: javac should constrain more uses of preview APIs
dc63584: 8273115: CountedLoopEndNode::stride_con crash in debug build with -XX:+TraceLoopOpts
c5aa75d: 8285439: remove unused os::fsync
9d9f4e5: 8285437: riscv: Fix MachNode size mismatch for MacroAssembler::verify_oops*
08024d9: 8193543: Regression automated test '/open/test/jdk/java/awt/TrayIcon/SystemTrayInstance/SystemTrayInstanceTest.java' fails
... and 94 more: https://git.openjdk.java.net/jdk/compare/21ea740e1da48054ee46efda493d0812a35d786e...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@ascarpino) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

ascarpino · 2022-04-18T15:13:14Z

x86-64 only uses this intrinsic, aarch64 does not support this intrinsic and uses the java code.

AlanBateman · 2022-04-18T15:53:04Z

x86-64 only uses this intrinsic, aarch64 does not support this intrinsic and uses the java code.

It looks like aarch64 has an implementation of generate_galoisCounterMode_AESCrypt, isn't that the code that implGCMCrypt0 runs?

ascarpino · 2022-04-18T16:21:28Z

Oh wow.. you're right. I never saw that come through.. thanks for seeing that. I'll run some tests that it's ok.

ascarpino

Please update the copyright year

ascarpino · 2022-04-18T23:45:43Z

I reran the test on aarch64 and x64 using -Xcomp without failure, also repeating the failing test. Given the bug says it was intermittent I can never been 100% certain it is perfect, but looking at the aarch64, it is just looping on that data it is presented, not hardcoded for a particular amount of data like the x64 code. That looping should work fine with the code change.
I think it's ready to go into the repo.

ascarpino

I'm not sure if the last commit removed the approval or not.. so i'll approve again

smita-kamath · 2022-04-25T17:07:55Z

/integrate

openjdk · 2022-04-25T17:09:43Z

@smita-kamath
Your change (at version 3c917fa) is now ready to be sponsored by a Committer.

ascarpino · 2022-04-25T22:43:59Z

/sponsor

openjdk · 2022-04-25T22:45:09Z

Going to push as commit 3416bfa.
Since your change was applied there have been 107 commits pushed to the master branch:

80a7f7b: 8267690: Revisit (Doc)Tree search implemented by throwing an exception
9b82708: 8284319: Test runtime/cds/appcds/TestParallelGCWithCDS.java fails in repo-loom
fb60594: 8285477: Add a PRECISION public static field to j.l.Float and j.l.Double
1e79ded: 8284889: runtime/cds/appcds/loaderConstraints/DynamicLoaderConstraintsTest.java#custom-cl-zgc timed out
414918d: 8285389: EdDSA trimming zeros
293bc5e: 8129778: Few awt test fail for Solaris 11 with RuntimeException
36f2e52: 8225777: java/awt/Mixing/MixingOnDialog.java fails on Ubuntu
32593df: 8279888: Local variable independently used by multiple loops can interfere with loop optimizations
4c22a9b: 8282823: javac should constrain more uses of preview APIs
dc63584: 8273115: CountedLoopEndNode::stride_con crash in debug build with -XX:+TraceLoopOpts
... and 97 more: https://git.openjdk.java.net/jdk/compare/21ea740e1da48054ee46efda493d0812a35d786e...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-04-25T22:45:30Z

@ascarpino @smita-kamath Pushed as commit 3416bfa.

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

Changed PARALLEL_LEN to 7680 as intrinsic processes multiples of 768 …

ca08ccd

…bytes

smita-kamath marked this pull request as ready for review April 18, 2022 05:07

openjdk bot added the rfr Pull request is ready for review label Apr 18, 2022

openjdk bot added the security security-dev@openjdk.org label Apr 18, 2022

ascarpino approved these changes Apr 18, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 18, 2022

ascarpino reviewed Apr 18, 2022

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 18, 2022

Updated copyright year

3c917fa

ascarpino approved these changes Apr 22, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 22, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 25, 2022

openjdk bot added the integrated Pull request has been integrated label Apr 25, 2022

openjdk bot closed this Apr 25, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 25, 2022

8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 #8280

8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 #8280

Uh oh!

Conversation

smita-kamath commented Apr 18, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 18, 2022

Uh oh!

openjdk bot commented Apr 18, 2022

Uh oh!

mlbridge bot commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman commented Apr 18, 2022

Uh oh!

openjdk bot commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascarpino commented Apr 18, 2022

Uh oh!

AlanBateman commented Apr 18, 2022

Uh oh!

ascarpino commented Apr 18, 2022

Uh oh!

ascarpino left a comment

Choose a reason for hiding this comment

Uh oh!

ascarpino commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ascarpino left a comment

Choose a reason for hiding this comment

Uh oh!

smita-kamath commented Apr 25, 2022

Uh oh!

openjdk bot commented Apr 25, 2022

Uh oh!

ascarpino commented Apr 25, 2022

Uh oh!

openjdk bot commented Apr 25, 2022

Uh oh!

openjdk bot commented Apr 25, 2022

Uh oh!

Uh oh!

smita-kamath commented Apr 18, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 18, 2022 •

edited

Loading

openjdk bot commented Apr 18, 2022 •

edited

Loading

ascarpino commented Apr 18, 2022 •

edited

Loading