8372816: New test sun/security/provider/acvp/ML_DSA_Intrinsic_Test.java succeeds in case of error

[vpaprotsk]

• Stop catching exception to make test able to fail
• Remove java-only test variation that cannot fail
• Reduce fuzz repeat count

---

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-8372816: New test sun/security/provider/acvp/ML_DSA_Intrinsic_Test.java succeeds in case of error (Bug - P3)

Reviewers

• Arno Zeller (@ArnoZeller - Author)
• Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28584

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vpaprotski! 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]

@vpaprotsk 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:

8372816: New test sun/security/provider/acvp/ML_DSA_Intrinsic_Test.java succeeds in case of error

Reviewed-by: azeller, mdoerr

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

• 5ea2b64: 8372977: unnecessary gthread-2.0 loading
• e534ee9: 8364343: Virtual Thread transition management needs to be independent of JVM TI
• ba777f6: 8372851: Modify java/io/File/GetXSpace.java to print path on failure of native call
• ... and 72 more: https://git.openjdk.org/jdk/compare/002fff39aace870b27a9068de1662fcb0b3033a6...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]

@vpaprotsk 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]

Webrevs

• 00: Full (41ab1ec7)

[ArnoZeller]

The change looks good, but you might consider using jdk.test.lib.RandomFactory. It allows to get and set a seed that is also printed at every run to make a failure easily reproducible.

And as the test now fails on aarch64 perhaps it should be excluded there and a bug should be opened - or is the behavior expected on aarch64 ?

[vpaprotsk]

The change looks good, but you might consider using jdk.test.lib.RandomFactory. It allows to get and set a seed that is also printed at every run to make a failure easily reproducible.

Thanks for the tip. Thats a pattern I use a lot! My complaint with RandomFactory is that now I need to add an extra classpath when calling the test directly. My development pattern is usually calling this command hundreds (thousands??) of times, until it is "convinced" to pass..

make images && ./build/linux-x86_64-server-fastdebug/images/jdk/bin/java -cp build/linux-x86_64-server-fastdebug/images/test/lib-test/test-lib.jar --add-opens java.base/sun.security.provider=ALL-UNNAMED --add-exports java.base/sun.security.provider=ALL-UNNAMED -XX:+UnlockDiagnosticVMOptions -XX:+UseDilithiumIntrinsics test/jdk/sun/security/provider/acvp/ML_DSA_Intrinsic_Test.java

(That is, I prefer to call java directly, instead of via jtreg)

And as the test now fails on aarch64 perhaps it should be excluded there and a bug should be opened - or is the behavior expected on aarch64 ?

I would prefer a separate bug report. There really should not be any failures on aarch64 either. But its entirely possible that I needed to constrain the range of RNG further (i.e. in which case this is test issue, not an actual bug in crypto..)

@ferakocz Would you mind having a look at aarch64 failure? I have no idea how its asm works.. I really hope its something like this fix I had to put in:

jdk/test/jdk/sun/security/provider/acvp/ML_DSA_Intrinsic_Test.java

Line 156 in 41ab1ec

int c = rnd.nextInt(dilithium_q);

(i.e. without that constraint, the int32 could overflow..)

PS: I am happy to help as I can with the range analysis.. I spent quite a bit agonizing over it in our own internal review.. Perhaps this could.. erm... "help" :D

I do not believe the end of the proof above is fully correct; particularly, I do not see how the leap to the range "highlighted in red" is possible.
I believe the conclusion is still valid, here is my own correction to that sentence:

[ArnoZeller]

Looks good to me but I'm no official "Reviewer".
The aarch64 issue can be solved in a different bug.

[TheRealMDoerr]

LGTM.

[vpaprotsk]

Thanks
/integrate

[openjdk]

Going to push as commit 70e2bc8.
Since your change was applied there have been 75 commits pushed to the master branch:

• 5ea2b64: 8372977: unnecessary gthread-2.0 loading
• e534ee9: 8364343: Virtual Thread transition management needs to be independent of JVM TI
• ba777f6: 8372851: Modify java/io/File/GetXSpace.java to print path on failure of native call
• ... and 72 more: https://git.openjdk.org/jdk/compare/002fff39aace870b27a9068de1662fcb0b3033a6...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vpaprotsk Pushed as commit 70e2bc8.

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

[dholmes-ora]

The test now fails in our CI

java.lang.RuntimeException: [Seed 3895491853102776133@5549] Result mult mismatch: fff0061f00128d23ffdbb433001c7a8a002d562effc43fe4001f6377001f7065ffeb1455ffd22289000872f8ffd1e03b0030157bffe420330031c9eb000138f8ffd7853f00172cf5ffd4a6d900377fabffc130e30033c31700168743ffcd3857001e6944fffc92d5002222ebffc240c7ffecb8d20027be25002a6fc7fff2d430002d63d800041a8e00329bfbffff19a3ffdc90fa00331163fff5b3be002e64e2002d94abffed410efff5f1f1ffc6ca8affed5c89ffd7ade3002f76b7003710c9ffec3d900032c19000078e9900144b62ffdf8ac800278c730027710a0037938affe96fd500180a2100040ea60011e0890026771c0009dc3fffdb18d6001280320036ea6a002606c0fffc59a6ffdbf812003948d80011b321ffdc21570040b2f800318a49fff70942001d2d85fff3afb5ffed3e6800350ea7ffdeff9f0029322cfff3fe4e003e4092ffec13d5ffdb300bffea957dfff6d551ffd4d6b6ffcd6298000a0f13ffdbb59effec6d6effca3d90000281de00342dccffc2b23c00020a84000f7e5d00204e73ffd74e630027873effec166cffe10d2c000a7633fff69dd40013d48a0040d1fe002a0b12002d9142000d8a34003611260011c7d0ffd0542e0018d4fd0020e664fff6b857ffe28094ffcdf56d00265624ffd1959bffed104f003d4c1e003d6514ffcaba43fffb42dbffffaac7ffdcf21affde30c3ffc1403500284751fffbdac3fff852caffc457a6002f84a40016a7dc00346fd0002bcc660025a001002ca06cfffd7d7a0005836c00196d51ffc54cb90032202dffcff2d0ffc20746fff65d800011b965ffd69a78ffe3ead3000af25a001fd2b6002aed57fff525cf0023acb4ffdc4505fff4a991ffe7eaf5003d3e7a0016101b0005edefffdaa2280010e4470020388b00243769ffeabebb0026f2910032ea28002bbadb00311a08001bfd3dfff57759ffcd969cffd9f38a0036519cffe06383003acb950017b420002ea308ffe45730ffbe3676ffcddd09ffeb6c8d003de2f5001c67caffdda044ffe29b4e000e443fffc71b240011e37bfff78b1300003622ffedee67ffca12b4ffeb43a4001cb086ffc48e8500083e48ffc094d2003dd646003d545dfffd10ea00014d88001c8c1d0015a16800124736ffeeb98dffc47dd000332948000420a00042f554ffdac256ffd787b4fff3cdfd00145203ffea1843ffe0301e00149d65001a9f5a0033e519001cacb2000fd3a500265d8d001eff1e000d29ceffe899160004328c000dd45afff54bb7ffc97978ffe81823ffcf53e1fff233afffc8d6350005d44500232f700008f9e2fff42792fff5f01000169917ffd72777fff39350ffd7b655ffeb282d000f2bab001db15effe3b005ffcc2ff2fffb2a26003beed6fffa0d66ffcaa34e000ba76d001f4238ffcd1b0fffd5de94000b264d != fff0061f00128d23ffdbb433001c7a8affad762dffc43fe4001f6377001f7065ffeb1455ffd22289000872f8ffd1e03b0030157bffe420330031c9eb000138f8ffd7853f00172cf5ffd4a6d9ffb79faaffc130e30033c31700168743ffcd3857001e6944fffc92d5002222ebffc240c7ffecb8d20027be25002a6fc7fff2d430002d63d800041a8e00329bfbffff19a3ffdc90fa00331163fff5b3be002e64e2002d94abffed410efff5f1f10046aa8bffed5c89ffd7ade3ffaf96b6003710c9ffec3d900032c19000078e9900144b62ffdf8ac800278c730027710a0037938affe96fd500180a2100040ea60011e0890026771c0009dc3fffdb18d6001280320036ea6a002606c0fffc59a6ffdbf812003948d80011b321ffdc21570040b2f800318a49fff70942001d2d85fff3afb5ffed3e6800350ea7ffdeff9f0029322cfff3fe4e003e4092ffec13d5ffdb300bffea957dfff6d551ffd4d6b6ffcd6298000a0f13ffdbb59effec6d6effca3d90000281de00342dccffc2b23c00020a84000f7e5d00204e73ffd74e630027873effec166cffe10d2c000a7633fff69dd40013d48a0040d1fe002a0b12002d9142000d8a34003611260011c7d0ffd0542e0018d4fd0020e664fff6b857ffe28094ffcdf56d002656240051759cffed104f003d4c1e003d6514ffcaba43fffb42dbffffaac7ffdcf21affde30c3ffc1403500284751fffbdac3fff852caffc457a6002f84a40016a7dc00346fd0002bcc660025a001002ca06cfffd7d7a0005836c00196d51ffc54cb90032202dffcff2d0ffc20746fff65d800011b965ffd69a78ffe3ead3000af25a001fd2b6002aed57fff525cf0023acb4ffdc4505fff4a991ffe7eaf5003d3e7a0016101b0005edefffdaa2280010e4470020388b00243769ffeabebb0026f2910032ea28002bbadb00311a08001bfd3dfff57759ffcd969cffd9f38a0036519cffe06383003acb950017b420002ea308ffe45730ffbe3676004dbd0affeb6c8d003de2f5001c67caffdda044ffe29b4e000e443fffc71b240011e37bfff78b1300003622ffedee670049f2b5ffeb43a4001cb086ffc48e8500083e48ffc094d2ffbdf645003d545dfffd10ea00014d88001c8c1d0015a16800124736ffeeb98dffc47dd000332948000420a00042f554ffdac256ffd787b4fff3cdfd00145203ffea1843ffe0301e00149d65001a9f5affb40518001cacb2000fd3a500265d8d001eff1e000d29ceffe899160004328c000dd45afff54bb7ffc97978ffe81823ffcf53e1fff233afffc8d6350005d44500232f700008f9e2fff42792fff5f01000169917ffd72777fff39350ffd7b655ffeb282d000f2bab001db15effe3b005ffcc2ff2fffb2a26003beed6fffa0d66ffcaa34e000ba76d001f4238ffcd1b0fffd5de94000b264d
	at ML_DSA_Intrinsic_Test.testMult(ML_DSA_Intrinsic_Test.java:143)
	at ML_DSA_Intrinsic_Test.main(ML_DSA_Intrinsic_Test.java:120)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
	at java.base/java.lang.Thread.run(Thread.java:1516)

[dholmes-ora]

FYI the Exception is redundant now you have added Throwable.

[vpaprotsk]

@dholmes-ora aarch64? might have to disable the test for it for now.. if its on x86_64.. then I need to investigate..

[dholmes-ora]

@vpaprotsk Aarch64. I'm filing a new bug

[vpaprotsk]

@dholmes-ora Will create a PR to disable it for AARCH for now, if you could approve, we could merge

[dholmes-ora]

Filed JDK-8373059

[vpaprotsk]

@dholmes-ora #28650

[ascarpino]

The integration of this change is unacceptable. The known aarch64 issue should have been resolved before integration, not deferred to another bug. If reviewers (@ArnoZeller @TheRealMDoerr) cannot verify other architectures, then the PR should not be approved unless the submitter is explicitly required to validate on all relevant platforms first.

[TheRealMDoerr]

The integration of this change is unacceptable. The known aarch64 issue should have been resolved before integration, not deferred to another bug. If reviewers (@ArnoZeller @TheRealMDoerr) cannot verify other architectures, then the PR should not be approved unless the submitter is explicitly required to validate on all relevant platforms first.

That was a bit too quick. Sorry for that. We should add the test to the problem list if we can't fix it quickly.

[ArnoZeller]

The integration of this change is unacceptable. The known aarch64 issue should have been resolved before integration, not deferred to another bug. If reviewers (@ArnoZeller @TheRealMDoerr) cannot verify other architectures, then the PR should not be approved unless the submitter is explicitly required to validate on all relevant platforms first.

Sorry for the to early approval and the issues caused by that!
There is a PR to disable the test on non X86: #28650

[vpaprotsk]

The integration of this change is unacceptable. The known aarch64 issue should have been resolved before integration, not deferred to another bug. If reviewers (@ArnoZeller @TheRealMDoerr) cannot verify other architectures, then the PR should not be approved unless the submitter is explicitly required to validate on all relevant platforms first.

Mea culpa.. was a bit too eager to push this off my plate. Should had thought before calling integrate.