Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
> Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. Leo Korinth has updated the pull request incrementally with one additional commit since the last revision: copyright - Changes: - all: https://git.openjdk.org/jdk/pull/15452/files - new: https://git.openjdk.org/jdk/pull/15452/files/4b2d1711..f3418c80 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15452&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15452&range=00-01 Stats: 344 lines in 344 files changed: 0 ins; 0 del; 344 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Re: RFR: 8314283: Support for NSS tests on aarch64 platforms
On Fri, 25 Aug 2023 13:02:39 GMT, Matthew Donovan wrote: > This PR updates the version of NSS to 3.91 and includes aarch64 platforms. > > There is a related bug in PR (https://github.com/openjdk/jdk/pull/15217) so > we may want to wait for that to merge before merging this one. If you have a local copy of NSS you can run the tests with jtreg instead of make. The tests expect a directory structure that looks like this: nss/ lib/ When I built NSS 3.91 locally, the final binaries were in `nss-3.91/dist/Debug/`. I created a directory, `nss`, and copied the bin, lib, and include directories into it: % ls $HOME/nss-3.91/dist/Debug/nss bin/ include/lib/ ``` I can run the test with jtreg by specifying the path in a system property: jtreg -verbose:summary -ea -esa -a -agentvm -conc:4 -report:executed \ -Djdk.test.lib.artifacts.nsslib-macosx_aarch64=$HOME/nss-3.91/dist/Debug \ test/jdk/sun/security/pkcs11/SampleTest.java The final component of the system property will depend on that platform you're running the test on and some of the jtreg arguments and paths will be different on your machine. - PR Comment: https://git.openjdk.org/jdk/pull/15429#issuecomment-1697295018
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 09:12:51 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > copyright I don't think this is the best change across so many files. It gives a very ugly name to a common test function and affects a very large number of tests. - Changes requested by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1600512718
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 14:06:01 GMT, Roger Riggs wrote: >> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> copyright > > I don't think this is the best change across so many files. > It gives a very ugly name to a common test function and affects a very large > number of tests. @RogerRiggs If it is only the name you want changed, maybe you can offer a better name? - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1697634872
Re: RFR: 8312428: PKCS11 tests fail with NSS 3.91 [v4]
On Fri, 25 Aug 2023 22:42:05 GMT, Valerie Peng wrote: >> Starting NSS v3.91, SHA-3 support is added for MessageDigest but not for PSS >> Signature. This breaks existing test assumptions made by PSS regression >> tests. In addition, the NSS SHA-3 message digests do not support cloning >> which causes the failure of TestCloning.java. >> >> This PR adds a PSSUtil.java class which provides utility method for >> detecting/guessing whether a digest algorithm is valid for PSS signature or >> not. >> >> The changes are verified with NSS v3.46, v3.57 and v3.91 (on local Linux >> machine). >> >> Thanks in advance for review~ > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > address more review feedbacks Marked as reviewed by rhalade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15217#pullrequestreview-1600868746
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 14:06:01 GMT, Roger Riggs wrote: >> Leo Korinth has updated the pull request incrementally with one additional >> commit since the last revision: >> >> copyright > > I don't think this is the best change across so many files. > It gives a very ugly name to a common test function and affects a very large > number of tests. > @RogerRiggs If it is only the name you want changed, maybe you can offer a > better name? @lkorinth Sorry for the too short comment; I wanted to make sure it wasn't integrated before I could look at it more closely. Neither the bug report or the PR describe the problem and its ramifications, only the solution. Can you elaborate on the conditions that lead to this. (and include them in the bug report). - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1697803842
Re: RFR: 8314283: Support for NSS tests on aarch64 platforms
On Fri, 25 Aug 2023 13:02:39 GMT, Matthew Donovan wrote: > This PR updates the version of NSS to 3.91 and includes aarch64 platforms. > > There is a related bug in PR (https://github.com/openjdk/jdk/pull/15217) so > we may want to wait for that to merge before merging this one. Please check lines of code at https://github.com/openjdk/jdk/pull/15429/files#diff-e49ff36652e24a0059feca1b9d68f2544ba609924e9ee25e01a90096532b66e5R709 added with [JDK-8296675](https://bugs.openjdk.org/browse/JDK-8296675). Should these be removed? - PR Comment: https://git.openjdk.org/jdk/pull/15429#issuecomment-1697817194
Re: RFR: 8315097: Rename createJavaProcessBuilder [v2]
On Tue, 29 Aug 2023 09:12:51 GMT, Leo Korinth wrote: >> Rename createJavaProcessBuilder so that it is not used by mistake instead of >> createTestJvm. >> >> I have used the following sed script: `find -name "*.java" | xargs -n 1 sed >> -i -e >> "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` >> >> Then I have manually modified ProcessTools.java. In that file I have moved >> one version of createJavaProcessBuilder so that it is close to the other >> version. Then I have added a javadoc comment in bold telling: >> >>/** >> * Create ProcessBuilder using the java launcher from the jdk to >> * be tested. >> * >> * Please observe that you likely should use >> * createTestJvm() instead of this method because createTestJvm() >> * will add JVM options from "test.vm.opts" and "test.java.opts" >> * and this method will not do that. >> * >> * @param command Arguments to pass to the java command. >> * @return The ProcessBuilder instance representing the java command. >> */ >> >> >> I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of >> the name of Utils.prependTestJavaOpts that adds those VM flags. If you have >> a better name I could do a rename of the method. I kind of like that it is >> long and clumsy, that makes it harder to use... >> >> I have run tier 1 testing, and I have started more exhaustive testing. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > copyright > I kind of like that it is long and clumsy, that makes it harder to use... ...but it's used in 462 files. That implies it is commonly used. Are you suggesting nearly all those uses are incorrect and eventually should be converted to `createTestJvm()`? - PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1698003837
Re: RFR: 8312306: Add more Reference.reachabilityFence() calls to the security classes using Cleaner
On Tue, 22 Aug 2023 18:06:59 GMT, Valerie Peng wrote: > This PR updates the various security classes using Cleaner with the > try/finally pattern. Also noticed that SunJCE's PBEKey impl class overrides > the destroy() method but not the isDestroyed() method, fixed this > inconsistency as well. > > Thanks in advance for the review! I am fine with the change - Marked as reviewed by ascarpino (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15390#pullrequestreview-1601310478
Re: RFR: 8309330: Allow java.security to be extended via a properties directory [v3]
> Currently, security properties are held within the `java.security` file in > the JDK tree for each installed JDK. The system property > `java.security.properties` can be used to point to a file containing > additional properties. These can be appended to the existing set or override > all existing properties. > > There is currently no way to specify additional properties permanently or to > reference multiple files. Making permanent changes to the `java.security` > properties requires editing the `java.security` file in each JDK where the > changes are required. > > This patch allows a directory tree to be specified either permanently in the > java.security file by the `security.propertiesDir` property or on the command > line using `java.security.propertiesDir`. Any property files found in this > directory tree can be appended to those specified in `java.security`, as with > the single file used by `java.security.properties`. > > As an example, the `security.propertiesDir` in the `java.security` file of > each JDK can be set to a common shared directory, allowing all JDKs to share > a common set of security properties. This eases setting up properties on each > new JDK installation and also allows the shared properties to be maintained > under different access permissions to those of the JDK. > > The command-line variant, `java.security.propertiesDir`, is intended > primarily for testing and to disable a permanent properties directory by > setting the value to empty. As with `java.security.properties`, the system > property will be ignored if `security.overridePropertiesFile` in the > `java.security` file is not set to true. > > A less flexible version of this patch (a permanent hardcoded single file) has > been [used in our JDK installations since > 2016](https://bugzilla.redhat.com/show_bug.cgi?id=1249083) to provide a > system-wide crypto policy. Having support for this in the upstream JDK would > allow us to remove a local patch from our builds and reduce divergence from > upstream. Andrew John Hughes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge remote-tracking branch 'jdk/master' into secdir - Sort the returned list of property files and exclude hidden files. - 8309330: Allow java.security to be extended via a properties directory - Changes: https://git.openjdk.org/jdk/pull/14277/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14277&range=02 Stats: 172 lines in 3 files changed: 163 ins; 2 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/14277.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14277/head:pull/14277 PR: https://git.openjdk.org/jdk/pull/14277
Integrated: 8312428: PKCS11 tests fail with NSS 3.91
On Thu, 10 Aug 2023 00:56:56 GMT, Valerie Peng wrote: > Starting NSS v3.91, SHA-3 support is added for MessageDigest but not for PSS > Signature. This breaks existing test assumptions made by PSS regression > tests. In addition, the NSS SHA-3 message digests do not support cloning > which causes the failure of TestCloning.java. > > This PR adds a PSSUtil.java class which provides utility method for > detecting/guessing whether a digest algorithm is valid for PSS signature or > not. > > The changes are verified with NSS v3.46, v3.57 and v3.91 (on local Linux > machine). > > Thanks in advance for review~ This pull request has now been integrated. Changeset: 1c598c22 Author:Valerie Peng URL: https://git.openjdk.org/jdk/commit/1c598c2245c5c348e946f4d0df653daa6e42da94 Stats: 268 lines in 4 files changed: 153 ins; 60 del; 55 mod 8312428: PKCS11 tests fail with NSS 3.91 Reviewed-by: ssahoo, rhalade - PR: https://git.openjdk.org/jdk/pull/15217
Re: RFR: 8309330: Allow java.security to be extended via a properties directory [v2]
On Thu, 24 Aug 2023 01:27:55 GMT, Martin Balao wrote: > As commented here [1], @franferrax and I have been working on a different > approach to this problem. We suggest to put this PR on hold until we have our > proposal ready to compare and discuss further. > > -- [1] - > https://bugs.openjdk.org/browse/JDK-8309331?focusedCommentId=14606264&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14606264 This doesn't seem to address the same problem. This PR & CSR are aimed at extending the existing functionality of `java.security.properties` with permanent configuration and the option of multiple files, while also allowing these configuration snippets to be maintained separately outside a single JDK. What you seem to be suggesting involves adding a meta-property to the property file to instruct it to be replaced with another file and would require multiple such instructions for each file. One of the benefits of this proposal is the directory can be setup once in the `java.security` file and then future maintenance can take place on the referenced directory for all JDKs. - PR Comment: https://git.openjdk.org/jdk/pull/14277#issuecomment-1698259042
Re: RFR: 8009550: PlatformPCSC should load versioned so
On Fri, 25 Aug 2023 05:39:39 GMT, Thomas Stuefe wrote: > Hi @gnu-andrew, > > in your last example, why does it look for both arm and x64 packages? And why > for kFreeBsd? I see you have both hardcoded, why? > > I would expect it only to attempt and pick up the architecture and OS the VM > was built for. Good question. Because they don't fit the template `$ARCH-linux-gnu/libpcsclite.so` which would expand to `arm-linux-gnu/libpcsclite.so`. I don't know of a way off-hand to get the ABI or the kernel in use (`kfreebsd` is not a BSD variant, but the usual GNU userland paired with the FreeBSD kernel rather than Linux). The [Wikipedia page](https://en.wikipedia.org/wiki/Debian#Derivatives_and_flavors) actually says it's now discontinued, so maybe we can just drop that one. It probably shows how long ago I [originally wrote and tested these paths](https://icedtea.wildebeest.org/hg/release/icedtea7-forest-2.6/jdk/rev/ae5765c7b8e2)... :) In short, that was my lazy option for catching those cases that won't fit the common one. I'm open to suggestions. We could skip any template with `'arm'` in, I guess, if the architecture doesn't match. It is worth noting though, that this file is already common to all the UNIX platforms and doesn't do any OS checks, despite the last check being a MacOS framework. That also presumably means MacOS doesn't have any of the `/usr` libraries in turn . > > Cheers, Thomas - PR Comment: https://git.openjdk.org/jdk/pull/15409#issuecomment-1698341466
Re: RFR: 8009550: PlatformPCSC should load versioned so
On Fri, 25 Aug 2023 00:39:14 GMT, Valerie Peng wrote: > Changes look fine. I submitted a mach5 test job just in case. Will approve > once the test job passes. BTW, I added a noreg-other label since there is no > regression test for this change. Thanks. Yes, I don't see how we can have a regression test for this one. My own testing was manual and involved moving the system library around as root... - PR Comment: https://git.openjdk.org/jdk/pull/15409#issuecomment-1698343166
Re: RFR: 8009550: PlatformPCSC should load versioned so
On Thu, 24 Aug 2023 00:53:03 GMT, Andrew John Hughes wrote:
> There is a long standing limitation in the UNIX smartcardio implementation
> which means it will only look for the `pcsclite` library in two locations;
> `/usr/lib` and `/usr/local/lib`. It also only searches for an unversioned
> library.
>
> On systems that separate libraries from development headers in separate
> subpackages, the unversioned library is only installed by the `-devel`
> package, which rarely happens unless the user wants to write their own
> application using the libpcsclite headers or build someone else's code
> themselves. An example of this is the [pcsc-lite-libs package on
> Fedora](https://koji.fedoraproject.org/koji/rpminfo?rpmID=35258843)
>
> On Debian-based systems, there is also the issue that libraries are now
> placed inside a subdirectory using a
> [tuple](https://wiki.debian.org/Multiarch/Tuples) consisting of the
> instruction set, syscall ABI and libc e.g. `x86_64-linux-gnu`. So it will
> not only fail to find the library when only the versioned library is
> installed (e.g.
> [libpcsclite1](https://packages.ubuntu.com/kinetic/amd64/libpcsclite1/filelist))
> but also when the dev package is also installed (e.g.
> [libpcsclite-dev](https://packages.ubuntu.com/kinetic/amd64/libpcsclite-dev/filelist)),
> because the file is in e.g. `/usr/lib/x86_64-linux-gnu/libpcsclite.so`
>
> This patch makes both cases work. It prefers versioned libraries, as
> potentially a `libpcsclite.so.2` could become available with a different ABI
> that the JDK can not work with. All cases that worked before with just
> `libpcsclite.so` will still work, but it should also now pick up the library
> on many additional systems.
>
> A much simpler patch (search for the versioned symlink instead of
> unversioned) has been [in the Fedora and RHEL packages for nearly a
> decade](https://bugzilla.redhat.com/show_bug.cgi?id=910107), and an earlier
> version of this patch with Debian support has been used in IcedTea builds
> since 2015. It would be nice to finally fix this upstream.
>
> Testing:
>
> Test case:
> ~~~
> import java.lang.reflect.Method;
>
> public class TestPlatformPCSC {
> public static void main(String[] args)
> throws Exception {
> Class cls = Class.forName("sun.security.smartcardio.PlatformPCSC");
> Method m = cls.getDeclaredMethod("getLibraryName");
> m.setAccessible(true);
> System.err.println(m.invoke(null));
> }
> }
> ~~~
>
> 1. With `libpcsclite.so.1` available:
> ~~~
> $ ~/builder/trunk/images/jdk/bin/java --add-opens
> java.smartcardio/sun.security.smartcardio=ALL-UNNAMED -Djava.security.d...
Marked as reviewed by stuefe (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/15409#pullrequestreview-1601963361
Re: RFR: 8009550: PlatformPCSC should load versioned so
On Wed, 30 Aug 2023 01:06:01 GMT, Andrew John Hughes wrote: >> Changes look fine. I submitted a mach5 test job just in case. Will approve >> once the test job passes. >> BTW, I added a noreg-other label since there is no regression test for this >> change. > >> Changes look fine. I submitted a mach5 test job just in case. Will approve >> once the test job passes. BTW, I added a noreg-other label since there is no >> regression test for this change. > > Thanks. Yes, I don't see how we can have a regression test for this one. My > own testing was manual and involved moving the system library around as > root... > > Hi @gnu-andrew, > > in your last example, why does it look for both arm and x64 packages? And > > why for kFreeBsd? I see you have both hardcoded, why? > > I would expect it only to attempt and pick up the architecture and OS the > > VM was built for. > > Good question. > > Because they don't fit the template `$ARCH-linux-gnu/libpcsclite.so` which > would expand to `arm-linux-gnu/libpcsclite.so`. > > I don't know of a way off-hand to get the ABI or the kernel in use > (`kfreebsd` is not a BSD variant, but the usual GNU userland paired with the > FreeBSD kernel rather than Linux). The [Wikipedia > page](https://en.wikipedia.org/wiki/Debian#Derivatives_and_flavors) actually > says it's now discontinued, so maybe we can just drop that one. It probably > shows how long ago I [originally wrote and tested these > paths](https://icedtea.wildebeest.org/hg/release/icedtea7-forest-2.6/jdk/rev/ae5765c7b8e2)... > :) > > In short, that was my lazy option for catching those cases that won't fit the > common one. I'm open to suggestions. We could skip any template with `'arm'` > in, I guess, if the architecture doesn't match. It is worth noting though, > that this file is already common to all the UNIX platforms and doesn't do any > OS checks, despite the last check being a MacOS framework. That also > presumably means MacOS doesn't have any of the `/usr` libraries in turn . Yes, that's a bit tricky. I was concerned about the JVM picking up the wrong library on a mulitarch system, since having multiple of these directories is the point of multiarch. But maybe its fine. The difference between the arm variants is that the float mode (soft vs hard) and I believe the former should always work, so if we accidentally pick it up, it should be no problem. The kfreebsd one I'd just drop. - PR Comment: https://git.openjdk.org/jdk/pull/15409#issuecomment-1698566452
