Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 30/06/21 12:10 pm, Alan Bateman wrote: On 30/06/2021 05:51, Jaikiran Pai wrote: In the case we are dealing with, the class is always "org.apache.tools.ant.types.Permissions". It will always be loaded by one single classloader (so classloaded just once). However, multiple different instances of this class will get created during the lifetime of the build and each such instance of org.apache.tools.ant.types.Permissions can/will invoke this setSecurityManager method. If I read this correctly, the caller of setSecurityManager is code in org.apache.tools.ant.types.Permission. There may be many instances of Permissions but from the perspective of setSecurityManager then it's all the same caller Class. Correct. -Jaikiran
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 30/06/2021 05:51, Jaikiran Pai wrote: In the case we are dealing with, the class is always "org.apache.tools.ant.types.Permissions". It will always be loaded by one single classloader (so classloaded just once). However, multiple different instances of this class will get created during the lifetime of the build and each such instance of org.apache.tools.ant.types.Permissions can/will invoke this setSecurityManager method. If I read this correctly, the caller of setSecurityManager is code in org.apache.tools.ant.types.Permission. There may be many instances of Permissions but from the perspective of setSecurityManager then it's all the same caller Class. -Alan
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 21:18:35 GMT, Weijun Wang wrote: >> Using a synchronized WeakHashMap with the class as the key would not prevent >> class unloading. >> Using a non-weak set or map to strings would keep the strings around for the >> life of the runtime. > > I hope this is uncommon but if that class is loaded by a `ClassLoader` again > and again then it will be different each time. I'll investigate more. WeakHashMap, Boolean>, where the key is the caller, should be okay (assume synchronization of course). Even with applications that do call setSecurityManager then the map is probably going to be one entry. I wouldn't expect it is common to create custom class loaders that load code that sets a security manager, meaning it is more likely that the container sets the SM rather have each plugin/application/whatever try to set the system-wide SM. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: Initial feedback from Apache Ant users on recent security manager warning messages
Hello Max, On 30/06/21 2:42 am, Wei-Jun Wang wrote: On Jun 29, 2021, at 3:08 AM, Jaikiran Pai wrote: On 29/06/21 12:34 pm, Jaikiran Pai wrote: Out of all these 4 points, I think if point number 2 can be addressed such that it just prints only once the warning for each caller class, then the issue noted by users of Ant build file will be drastically reduced. I haven't yet tried or proved it, but I think we will end up with just one log message in their STDERR for the entire build for a majority of the cases. I will experiment with that this week to see if that's true. FWIW - I implemented a very primitive cache in the System class to log this message just once and see if it helps for the general/regular usecase that I used as an example. To be clear, just once for each caller class. In your case, is the class always the same object? I was thinking about if I should use a String or a Class as the map key. Is it possible for the same class to be loaded again and again? In the case we are dealing with, the class is always "org.apache.tools.ant.types.Permissions". It will always be loaded by one single classloader (so classloaded just once). However, multiple different instances of this class will get created during the lifetime of the build and each such instance of org.apache.tools.ant.types.Permissions can/will invoke this setSecurityManager method. One additional detail - this org.apache.tools.ant.types.Permissions is _not_ final, which means it can potentially have sub-classes. In the context of Ant, the fact that this class is not final shouldn't really have any implication in how the caching in java.lang.System gets implemented. I only bring this up here because maybe it has to be taken into account for the general cases? I haven't yet checked how the Reflection.getCallerClass() is implemented in the JDK and whether this caching logic that's being introduced needs to take into account sub-classes while deciding whether or not to issue a warning. -Jaikiran
Re: RFR: Merge jdk17 [v2]
> Forwardport JDK 17 -> JDK 18 Jesper Wilhelmsson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 113 commits: - Merge - 8269615: Fix for 8263640 broke Windows build Reviewed-by: iklam, dcubed - 8269268: JDWP: Properly fix thread lookup assert in findThread() Reviewed-by: kevinw, amenkov, sspitsyn - 8260540: serviceability/jdwp/AllModulesCommandTest.java failed with "Debuggee error: 'ERROR: transport error 202: bind failed: Address already in use'" Reviewed-by: sspitsyn, kevinw - 8263640: hs_err improvement: handle class path longer than O_BUFLEN Reviewed-by: iklam, minqi, dholmes - 8269417: Minor clarification on NonblockingQueue utility Reviewed-by: kbarrett, iwalulya - 8269530: runtime/ParallelLoad/ParallelSuperTest.java timeout Reviewed-by: dholmes, coleenp - 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC Reviewed-by: iwalulya, kbarrett - 8261579: AArch64: Support for weaker memory ordering in Atomic Reviewed-by: adinn, shade - 8268821: Split systemDictionaryShared.cpp Reviewed-by: erikj, ccheung, iklam - ... and 103 more: https://git.openjdk.java.net/jdk/compare/0d745ae8...be964f2e - Changes: https://git.openjdk.java.net/jdk/pull/4630/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4630&range=01 Stats: 30219 lines in 619 files changed: 17676 ins; 10608 del; 1935 mod Patch: https://git.openjdk.java.net/jdk/pull/4630.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4630/head:pull/4630 PR: https://git.openjdk.java.net/jdk/pull/4630
Integrated: Merge jdk17
On Wed, 30 Jun 2021 00:31:34 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: ee526a2e Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/ee526a2ea840aedb97b23538f9d624acbccebc97 Stats: 127 lines in 11 files changed: 70 ins; 17 del; 40 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4630
Re: RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server
On Thu, 17 Jun 2021 13:20:54 GMT, Alexey Bakhtin wrote: > Please review the fix for JDK-8268965. > > The new jtreg test is added for the described issue. > sun/security/ssl and javax/net/ssl tests are passed I would like to have a look. I was wondering if there is a racing between close and read and if the closure could be hang. But I have no time to think more about them currently. Hopefully, I could have some cycles in the next couple of weeks. - PR: https://git.openjdk.java.net/jdk/pull/4520
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18
-
Commit messages:
- Merge
- 8269034: AccessControlException for SunPKCS11 daemon threads
- 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails
in Windows debug build
- 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
- 8268884: C2: Compile::remove_speculative_types must iterate top-down
- 8249646: Runtime.exec(String, String[], File) documentation contains literal
{@link ...}
- 8268699: Shenandoah: Add test for JDK-8268127
- 8269517: compiler/loopopts/TestPartialPeelingSinkNodes.java crashes with
-XX:+VerifyGraphEdges
- 8269126: Rename G1AllowPreventiveGC option to G1UsePreventiveGC
The merge commit only contains trivial merges, so no merge-specific webrevs
have been generated.
Changes: https://git.openjdk.java.net/jdk/pull/4630/files
Stats: 127 lines in 11 files changed: 70 ins; 17 del; 40 mod
Patch: https://git.openjdk.java.net/jdk/pull/4630.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4630/head:pull/4630
PR: https://git.openjdk.java.net/jdk/pull/4630
[jdk17] Integrated: 8269034: AccessControlException for SunPKCS11 daemon threads
On Tue, 22 Jun 2021 13:26:41 GMT, Sean Coffey wrote: > Sufficient permissions missing if this code was ever to run with > SecurityManager. > > Cleanest approach appears to be use of InnocuousThread to create the > cleaner/poller threads. > Test case coverage extended to cover the SecurityManager scenario. > > Reviewer request: @valeriepeng This pull request has now been integrated. Changeset: 0d745ae8 Author:Sean Coffey URL: https://git.openjdk.java.net/jdk17/commit/0d745ae8fde5cab290dc8c695d2906f9a98c491c Stats: 95 lines in 5 files changed: 53 ins; 17 del; 25 mod 8269034: AccessControlException for SunPKCS11 daemon threads Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk17/pull/117
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:57:24 GMT, Roger Riggs wrote: >> If I switch to a "non-weak" set or map, then it seems I can safely use the >> source string as the key. Will using the Class object as a key prevent them >> from unloading? > > Using a synchronized WeakHashMap with the class as the key would not prevent > class unloading. > Using a non-weak set or map to strings would keep the strings around for the > life of the runtime. I hope this is uncommon but if that class is created by a `ClassLoader` again and again then it will be different each time. I'll investigate more. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: Initial feedback from Apache Ant users on recent security manager warning messages
> On Jun 29, 2021, at 3:08 AM, Jaikiran Pai wrote: > > > On 29/06/21 12:34 pm, Jaikiran Pai wrote: >> >> >> >> > Out of all these 4 points, I think if point number 2 can be addressed such >> > that it just prints only once the warning for each caller class, then the >> > issue noted by users of Ant build file will be drastically reduced. I >> > haven't yet tried or proved it, but I think we will end up with just one >> > log message in their STDERR for the entire build for a majority of the >> > cases. I will experiment with that this week to see if that's true. >> >> FWIW - I implemented a very primitive cache in the System class to log this >> message just once and see if it helps for the general/regular usecase that I >> used as an example. > > To be clear, just once for each caller class. In your case, is the class always the same object? I was thinking about if I should use a String or a Class as the map key. Is it possible for the same class to be loaded again and again? Thanks, Max > > -Jaikiran >
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:35:40 GMT, Weijun Wang wrote: >> Using a HashSet could use the callerClass as the key and be a stable >> reference for having given the message. >> or use a ConcurrentHashMap>, boolean> and avoid any separate >> synchronization that would be needed with a HashSet or HashMap. > > If I switch to a "non-weak" set or map, then it seems I can safely use the > source string as the key. Will using the Class object as a key prevent them > from unloading? Using a synchronized WeakHashMap with the class as the key would not prevent class unloading. Using a non-weak set or map to strings would keep the strings around for the life of the runtime. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On 06/29/21 15:39, Weijun Wang wrote: > If I switch to a "non-weak" set or map, then it seems I can safely use > the source string as the key. Will using the Class object as a key prevent > them from unloading? I understand that it's in principle possible for classes to get GC'd, though I don't know how often it happens in typical practical settings. This seems like a question to be approached from the angle of asking what the wanted semantics should be. What should be the effect on warning messages if class a.b.C is loaded and sets a security manager, and is later GC'd, and then a class also named a.b.C is later loaded, and sets a security manager? It seems like a preferred answer to that question would determine whether to make the set weak or not. Regards, -Chap
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 19:23:26 GMT, Roger Riggs wrote: >> src/java.base/share/classes/java/lang/System.java line 337: >> >>> 335: = Collections.synchronizedMap(new WeakHashMap<>()); >>> 336: } >>> 337: >> >> I wonder about the use of a WeakHashMap here. That may work well when the >> source is an interned string (a class name) which will be strongly >> referenced elsewhere and may be garbage collected if the class is unloaded, >> but in the case where it contains the name of the source jar then that >> string will only be referenced by the weak hashmap, and therefore it could >> be garbage collected any time - which would cause the mapping to be removed. >> In that case you cannot guarantee that the warning will be emitted only once. > > Using a HashSet could use the callerClass as the key and be a stable > reference for having given the message. > or use a ConcurrentHashMap>, boolean> and avoid any separate > synchronization that would be needed with a HashSet or HashMap. If I switch to a "non-weak" set or map, then it seems I can safely use the source string as the key. Will using the Class object as a key prevent them from unloading? - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Tue, 29 Jun 2021 18:47:23 GMT, Daniel Fuchs wrote: >> Add a cache to record which sources have called `System::setSecurityManager` >> and only print out warning lines once for each. > > src/java.base/share/classes/java/lang/System.java line 337: > >> 335: = Collections.synchronizedMap(new WeakHashMap<>()); >> 336: } >> 337: > > I wonder about the use of a WeakHashMap here. That may work well when the > source is an interned string (a class name) which will be strongly referenced > elsewhere and may be garbage collected if the class is unloaded, but in the > case where it contains the name of the source jar then that string will only > be referenced by the weak hashmap, and therefore it could be garbage > collected any time - which would cause the mapping to be removed. In that > case you cannot guarantee that the warning will be emitted only once. Using a HashSet could use the callerClass as the key and be a stable reference for having given the message. or use a ConcurrentHashMap>, boolean> and avoid any separate synchronization that would be needed with a HashSet or HashMap. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller
On Mon, 28 Jun 2021 21:09:43 GMT, Weijun Wang wrote: > Add a cache to record which sources have called `System::setSecurityManager` > and only print out warning lines once for each. src/java.base/share/classes/java/lang/System.java line 337: > 335: = Collections.synchronizedMap(new WeakHashMap<>()); > 336: } > 337: I wonder about the use of a WeakHashMap here. That may work well when the source is an interned string (a class name) which will be strongly referenced elsewhere and may be garbage collected if the class is unloaded, but in the case where it contains the name of the source jar then that string will only be referenced by the weak hashmap, and therefore it could be garbage collected any time - which would cause the mapping to be removed. In that case you cannot guarantee that the warning will be emitted only once. - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v3]
On Tue, 29 Jun 2021 00:07:41 GMT, Sean Coffey wrote: >> Sufficient permissions missing if this code was ever to run with >> SecurityManager. >> >> Cleanest approach appears to be use of InnocuousThread to create the >> cleaner/poller threads. >> Test case coverage extended to cover the SecurityManager scenario. >> >> Reviewer request: @valeriepeng > > Sean Coffey 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 four additional > commits since the last revision: > > - Edits from review > - Merge remote-tracking branch 'origin/master' into pkcs11-perms > - Move TokenPoller to Runnable > - 8269034: AccessControlException for SunPKCS11 daemon threads Update looks good. Thanks, Valerie - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/117
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 29/06/2021 08:04, Jaikiran Pai wrote: Thank you Alan. I see that https://bugs.openjdk.java.net/browse/JDK-8269543 has been created for this. I'll keep a watch on that one. Yes, that was the original intention but had to dropped because it wasn't thread safe. BTW: I'm puzzled as to why Ant does this. I re-read your mail yesterday a few times and it seems a real outlier to run an Ant script like this. Maybe there are use-cases beyond building where this may be have been useful in the past? (I understand you may not have all the history/rational for this). -Alan
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 29/06/21 12:34 pm, Jaikiran Pai wrote: > Out of all these 4 points, I think if point number 2 can be addressed such that it just prints only once the warning for each caller class, then the issue noted by users of Ant build file will be drastically reduced. I haven't yet tried or proved it, but I think we will end up with just one log message in their STDERR for the entire build for a majority of the cases. I will experiment with that this week to see if that's true. FWIW - I implemented a very primitive cache in the System class to log this message just once and see if it helps for the general/regular usecase that I used as an example. To be clear, just once for each caller class. -Jaikiran
Re: Initial feedback from Apache Ant users on recent security manager warning messages
On 29/06/21 12:21 am, Alan Bateman wrote: On 28/06/2021 18:16, Jaikiran Pai wrote: On a slightly related note, I was wondering why we decided to go with what appears to be a bit more aggressive approach to these warning messages as compared to what was done with the illegal reflective access warnings? I would have thought that the illegal reflective access changes would be much more involved if not the same level as moving away from setSecurityManager calls. The typical SM setup will be to set it once, the Ant "same JVM" scenario where it sets and then resets it may be unusual. In any case, the original implementation patch did have caching to avoid duplicates. It wasn't quite right and had to be pulled, it may be time to re-visit that to avoid too much noise for code that sets it many times. Thank you Alan. I see that https://bugs.openjdk.java.net/browse/JDK-8269543 has been created for this. I'll keep a watch on that one. > Out of all these 4 points, I think if point number 2 can be addressed such that it just prints only once the warning for each caller class, then the issue noted by users of Ant build file will be drastically reduced. I haven't yet tried or proved it, but I think we will end up with just one log message in their STDERR for the entire build for a majority of the cases. I will experiment with that this week to see if that's true. FWIW - I implemented a very primitive cache in the System class to log this message just once and see if it helps for the general/regular usecase that I used as an example. That indeed helped and I see that the warning message just gets printed once for the entire lifetime of the build process, no matter how many "java" tasks get called. So the changes in JDK-8269543 will help us to a considerable extent. -Jaikiran
