On Thu, 4 Sep 2025 15:37:57 GMT, Mikhail Yankelevich <[email protected]>
wrote:
>> This PR updates PKCS11 tests to better handle NSS version numbers. The
>> previous code treated the version numbers as double values and used
>> comparison operators. The problem is that it incorrectly treats 3.111 as
>> between 3.11 and 3.12. This update parses and compares the major and minor
>> version numbers separately.
>
> test/jdk/sun/security/pkcs11/Cipher/TestKATForGCM.java line 330:
>
>> 328: String osName = System.getProperty("os.name");
>> 329: int idx = ver.indexOf(".");
>> 330: double major = Double.parseDouble(ver.substring(0,
>> idx));
>
> IMO the split between major version and minor is a bit hard to read. Wouldn't
> it be easier to just get a major.minor version entirely with something like:
>
>
> String[] splitParts = ver.split("//.");
> Double.parseDouble(splitParts.length > 1
> ? splitParts[0] + "." + splitParts[1]
> : splitParts[0]);
>
>
> This way it will always take a full double value (1.13 will not be the same
> as 1.1, as it's now as far as I can see) and would be a bit easier to
> understand
>
> The checking for only major version could be either `doubleVersion<4 &&
> doubleVersion>=3` or even cleaner, using floor function
> `Math.floor(doubleVersion)`
>
> And the same for the other files.
>
> What do you think?
I may be misunderstanding your comment but it looks like you're using a single,
`double` value to compare versions. The original code did that but it doesn't
work in all cases. The problem occurs when checking a range such as
`version >= 3.11 && version < 3.12`
If the version number is 3.111, then the comparison is `true` and tests are
skipped, even though _version_ 3.111 is "greater" than 3.12. So this code
creates two doubles: `major` and `minor` to do separate comparisons of the
values.
To further complicate things, NSS also has versions of the form `x.y.z`. The
original code combined the 'y' and 'z' components to go from 3.11.9 to 3.119.
My change would create major version 3.0 and minor version 11.9.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27095#discussion_r2322797235
