On Thu, 6 Jun 2024 15:45:54 GMT, Fernando Guallini <fguall...@openjdk.org> wrote:
>> As highlighted in the bug description, The test >> **security/Security/SynchronizedAccess.java** have some issues: >> >> 1. it needs to implement the sigalg, otherwise it throws >> java.security.NoSuchAlgorithmException . Even though it does not fail as it >> catches the exception, it never reaches the removeProvider loop. Also, >> adding an extra assertion to verify the local providers were actually >> removed. >> 2. It is reassigning the **provs** variable with Security.getProviders(). >> This will start adding/removing some of the system providers, in addition to >> the local providers, which it is not the test intent. >> 3. Adding othervm flag to run in isolation. >> >> Test runs in tier2 > > Fernando Guallini has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - added brackets, and refactor a comment and an exception message > - Regtest java/security/Security/SynchronizedAccess.java is incorrect. There's a bunch of issues in this test which I think we can better handle by a slight rewrite and approach in our testing. In the original bug [JDK-4162583](https://bugs.openjdk.org/browse/JDK-4162583): > The various methods on java.security.Security for inserting, removing and looking up providers and provider properties are not synchronized. If providers are inserted or removed by one thread while lookups are occurring in another thread then spurious failures such as null pointer exceptions or bad array indexing may occur. The intent of this test was to do a bunch of provider creations/insertions/removals and Signature creations, and make sure that we don't end up with any unexpected `NPE`/`IOOBE`/etc. possibly due to invalid states. Due to several issues in the original test, many of these methods didn't even get exercised (or maybe did in older JDK versions where side-effects were different?), but the test should be updated to be correct and meet today's semantics. I might suggest trying something like the [attached test](https://github.com/user-attachments/files/16257435/SynchronizedAccess.java.patch), which is very similar in approach to the existing test, but will test additional cases: 1. checks the number of providers before/after running to make sure no extra providers remain hanging around at the conclusion, 2. we force the interspersion of `addProvider()`/`getInstance()`/`removeProvider()` with `Thread.yield()`. 3. each thread tries to repeatedly `addProvider()`/`Signature.getInstance()`/`removeProvider()` with 10 providers with the same names. 4. many of these `addProvider()` calls will fail because the provider names are the same (silent return value -1). However, some number will pass (return value >= 0) 5. the `Signature.getInstance()` may fail with a `NSAE` depending on the provider status/call ordering, or will succeed, depending on the provider availability (add/remove). 6. we always try to remove the providers, some of which will silently fail because a provider with a similar name has already been removed. What do you think? test/jdk/java/security/Security/SynchronizedAccess.java line 26: > 24: /* > 25: * @test > 26: * @bug 4162583 7054918 8130181 Add `8028127` :) test/jdk/java/security/Security/SynchronizedAccess.java line 29: > 27: * @library /test/lib ../testlibrary > 28: * @summary Make sure Provider api implementations are synchronized > properly > 29: * @run main/othervm SynchronizedAccess There are two options here. You can either: 1. use `/othervm` and remove the `ProvidersSnapshot`. 2. Keep the `ProvidersSnapshot` and use `/othervm`. The `ProvidersSnapshot` **SHOULD** return the provider list in this agent VM back to the original state before releasing it to the agentvm for the next test. That was the point of [JDK-7054918](https://bugs.openjdk.org/browse/JDK-7054918). If we were concerned that the test could somehow exit without hitting the finally (unlikely), then the second option (`/othervm`) would be a safer alternative. test/jdk/java/security/Security/SynchronizedAccess.java line 82: > 80: } > 81: Signature.getInstance("sigalg"); > 82: // Skip the first provider to ensure one is always > available for getInstance. This is forcing a successful `getInstance()`: you might want to check for a failures too. See suggested fix attachment in the overall comments. ------------- PR Review: https://git.openjdk.org/jdk/pull/19480#pullrequestreview-2178942456 PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1678580046 PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1678591666 PR Review Comment: https://git.openjdk.org/jdk/pull/19480#discussion_r1680153336
