Re: JDK 19 innocuous reaper threads
Thanks you Alan and Roger. I filed the following issue to track this: https://bugs.openjdk.org/browse/JDK-8297451 There are likely many more usages of innocuous thread that could be improved by ensuring setDaemon is invoked while asserting privileges, but I'd like to leave those to a later issue, since the usage in process is causing us issues right now, and it would be great to get this fixed (and eventually backported to 19.0.2). -Chris. On 22/11/2022 17:01, Roger Riggs wrote: Hi Chris, Yes, adding a doPriv for setDaemon and setName in a couple of places makes sense. Thanks, Roger On 11/22/22 11:12 AM, Chris Hegarty wrote: Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: JDK 19 innocuous reaper threads
Hi Chris, Yes, adding a doPriv for setDaemon and setName in a couple of places makes sense. Thanks, Roger On 11/22/22 11:12 AM, Chris Hegarty wrote: Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: JDK 19 innocuous reaper threads
Hi Alan, On 22/11/2022 16:08, Alan Bateman wrote: On 22/11/2022 15:21, Chris Hegarty wrote: .. Just to double check, does the ES security manager override checkAccess(Thread)? Yes. :-( That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. Right. That's exactly what we're running into. If there are no objections, then I'm happy to file an issue and PR to add narrow doPriv blocks around these calls. -Chris [1] https://github.com/elastic/elasticsearch/blob/main/libs/secure-sm/src/main/java/org/elasticsearch/secure_sm/SecureSM.java#L118
Re: JDK 19 innocuous reaper threads
On 22/11/2022 15:21, Chris Hegarty wrote: Hi, A change in JDK 19, that changed process reaper threads to be innocuous [1], has had an adverse affect when terminating the Elasticsearch server [2]. I agree with changing the process reapers to be innocuous, but just wonder if we're missing a few doPriv blocks. Additionally, and also in JDK 19, is the welcome improvement of the pid in the reaper thread name [3], but again I wonder if we're missing a few doPriv blocks here too. The issues we're seeing arise from the calls to `setDaemon` and `setName` outside of doPriv blocks, which can (depending on your security manager implementation) result in checking the callers permissions, and its callers permissions, etc, all the way to the thread's inherited access control context - which is effectively empty for these threads, since they are innocuous. I don't remember where we ended up with these kinda calls for innocuous threads, I'm thinking specifically about `setDaemon` (but `setName` seems similar) - whether they should be in doPriv or not, given the default security manager implementation. My feeling is that they should, since the caller should never be required to hold any specific permissions for these specific operations [4][5][6] to succeed. Just to double check, does the ES security manager override checkAccess(Thread)? That is usually a no-op but if overridden then it will expose an issue with the thread factory for the "process reaper" where it attempts changes the daemon status outside of a doPriv block. -Alan
JDK 19 innocuous reaper threads
Hi, A change in JDK 19, that changed process reaper threads to be innocuous [1], has had an adverse affect when terminating the Elasticsearch server [2]. I agree with changing the process reapers to be innocuous, but just wonder if we're missing a few doPriv blocks. Additionally, and also in JDK 19, is the welcome improvement of the pid in the reaper thread name [3], but again I wonder if we're missing a few doPriv blocks here too. The issues we're seeing arise from the calls to `setDaemon` and `setName` outside of doPriv blocks, which can (depending on your security manager implementation) result in checking the callers permissions, and its callers permissions, etc, all the way to the thread's inherited access control context - which is effectively empty for these threads, since they are innocuous. I don't remember where we ended up with these kinda calls for innocuous threads, I'm thinking specifically about `setDaemon` (but `setName` seems similar) - whether they should be in doPriv or not, given the default security manager implementation. My feeling is that they should, since the caller should never be required to hold any specific permissions for these specific operations [4][5][6] to succeed. -Chris. [1] https://bugs.openjdk.org/browse/JDK-8279488 [2] https://github.com/elastic/elasticsearch/issues/91650 [3] https://bugs.openjdk.org/browse/JDK-8284165 [4] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#L103 [5] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL144 [6] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ProcessHandleImpl.java#LL175
