On Tue, 15 Apr 2025 15:09:16 GMT, Ferenc Rakoczi <d...@openjdk.org> wrote:
>> @adinn Hi, Andrew, >> I think I addressed all of your comment improvement comments, in most cases >> I just changed them as you suggested. Thanks a lot for the thorough review! > >> @ferakocz >> >> Hi Ferenc, >> >> Sorry, but I still had a few comments to add to the KyberNTTMult routine to >> clarify exactly how the load, compute and store operations relate to the >> original Java source. That's the only remaining code that I felt needed >> further clarification for maintainers. So, after you work through them I can >> approve the PR. > > No problem , it was easy to make the changes. Thanks again! > @ferakocz I reran test > jtreg:test/jdk/sun/security/provider/acvp/Launcher.java and hit a Java > assertion: > > ``` > >> ML-KEM-512 encapsulation > 1 STDERR: > java.lang.AssertionError > at > java.base/com.sun.crypto.provider.ML_KEM.twelve2Sixteen(ML_KEM.java:1371) > at java.base/com.sun.crypto.provider.ML_KEM.decodePoly(ML_KEM.java:1408) > at > java.base/com.sun.crypto.provider.ML_KEM.decodeVector(ML_KEM.java:1337) > at java.base/com.sun.crypto.provider.ML_KEM.kPkeEncrypt(ML_KEM.java:712) > at java.base/com.sun.crypto.provider.ML_KEM.encapsulate(ML_KEM.java:555) > at > java.base/com.sun.crypto.provider.ML_KEM_Impls$K.implEncapsulate(ML_KEM_Impls.java:134) > at > java.base/sun.security.provider.NamedKEM$KeyConsumerImpl.engineEncapsulate(NamedKEM.java:124) > at java.base/javax.crypto.KEM$Encapsulator.encapsulate(KEM.java:265) > at java.base/javax.crypto.KEM$Encapsulator.encapsulate(KEM.java:225) > at ML_KEM_Test.encapDecapTest(ML_KEM_Test.java:98) > at ML_KEM_Test.run(ML_KEM_Test.java:41) > at Launcher.run(Launcher.java:160) > at Launcher.main(Launcher.java:122) > 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:1447) > > JavaTest Message: Test threw exception: java.lang.AssertionError > JavaTest Message: shutting down test > ``` > > The offending code is this: > > ``` > private void twelve2Sixteen(byte[] condensed, int index, > short[] parsed, int parsedLength) { > int i = parsedLength / 64; > int remainder = parsedLength - i * 64; > if (remainder != 0) { > i++; > } > assert (((remainder != 0) && (remainder != 48)) || <== assert here > index + i * 96 > condensed.length); > ``` > > I believe the logic is reversed here i.e. it should be: > > ``` > assert ((remainder == 0) || (remainder == 48)) && > index + i * 96 <= condensed.length); > ``` > > Does that sound right? Aarrrrgh, yes. I forgot to negate that condition when I went from throwing an exception to assert, and I also thought, incorrectly, that -ea would enable my assertions when I tested :-( . Thanks a lot for catching it! ------------- PR Comment: https://git.openjdk.org/jdk/pull/23663#issuecomment-2807096779