Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
Thanks Alan. Updates made and changes pushed. regards, Sean. On 13/01/2020 18:50, Alan Bateman wrote: On 13/01/2020 10:28, Seán Coffey wrote: some off line comments suggested that I could move the jar initialization checks to the EventHelper class. With that in place, the EventHelper utility class should never initialize the logging framework early during jar initialization. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/ Thanks for the update. JAR file verification is tricky and important not to attempt to run arbitrary code while doing that, esp. anything that might need to load a class or resource from the class path. So I think the approach (in v5) looks okay. A minor nit in JarFile is that it should be "static final". Also you might want to replace or change the @summary in both tests to make it clearer that the tests attempt to trigger class loading from the class loader during JAR file verification. -Alan.
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
On 13/01/2020 10:28, Seán Coffey wrote: some off line comments suggested that I could move the jar initialization checks to the EventHelper class. With that in place, the EventHelper utility class should never initialize the logging framework early during jar initialization. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/ Thanks for the update. JAR file verification is tricky and important not to attempt to run arbitrary code while doing that, esp. anything that might need to load a class or resource from the class path. So I think the approach (in v5) looks okay. A minor nit in JarFile is that it should be "static final". Also you might want to replace or change the @summary in both tests to make it clearer that the tests attempt to trigger class loading from the class loader during JAR file verification. -Alan.
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
On 13/01/2020 17:21, Chris Hegarty wrote: On 13 Jan 2020, at 17:19, Seán Coffey wrote: Thanks for the reviews. All callers of EventHelper log methods are ensuring that isLoggingSecurity() is true before proceeding. I've added an assert null check in the 4 logger methods to ensure expectations are in place. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/ Thanks. LGTM. Likewise. Thanks Seán! best regards, -- daniel
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
> On 13 Jan 2020, at 17:19, Seán Coffey wrote: > > Thanks for the reviews. All callers of EventHelper log methods are ensuring > that isLoggingSecurity() is true before proceeding. I've added an assert null > check in the 4 logger methods to ensure expectations are in place. > > http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/ Thanks. LGTM. -Chris.
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
Thanks for the reviews. All callers of EventHelper log methods are ensuring that isLoggingSecurity() is true before proceeding. I've added an assert null check in the 4 logger methods to ensure expectations are in place. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/ Hope this helps, Sean. On 13/01/2020 14:38, Daniel Fuchs wrote: On 13/01/2020 14:06, Chris Hegarty wrote: I’m going to ask, since I cannot find the answer myself. Why are some securityLogger::log invocations guarded with isLoggingSecurity, and others not? With this change shouldn’t all invocations be guarded, since it is isLoggingSecurity that assigns securityLogger a value? Argh! Well spotted chris! -- daniel
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
On 13/01/2020 14:06, Chris Hegarty wrote: I’m going to ask, since I cannot find the answer myself. Why are some securityLogger::log invocations guarded with isLoggingSecurity, and others not? With this change shouldn’t all invocations be guarded, since it is isLoggingSecurity that assigns securityLogger a value? Argh! Well spotted chris! -- daniel
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
> On 13 Jan 2020, at 13:14, Daniel Fuchs wrote: > > On 13/01/2020 10:28, Seán Coffey wrote: >> some off line comments suggested that I could move the jar initialization >> checks to the EventHelper class. With that in place, the EventHelper utility >> class should never initialize the logging framework early during jar >> initialization. >> http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/ > > Looks good to me Seán. Probably safer than the other alternatives. +1 I’m going to ask, since I cannot find the answer myself. Why are some securityLogger::log invocations guarded with isLoggingSecurity, and others not? With this change shouldn’t all invocations be guarded, since it is isLoggingSecurity that assigns securityLogger a value? -Chris
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
On 13/01/2020 10:28, Seán Coffey wrote: some off line comments suggested that I could move the jar initialization checks to the EventHelper class. With that in place, the EventHelper utility class should never initialize the logging framework early during jar initialization. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/ Looks good to me Seán. Probably safer than the other alternatives. 46 private static boolean loggingSecurity; that should be volatile too. best regard, -- daniel regards, Sean. On 16/12/2019 14:15, Seán Coffey wrote: The recent crypto event logging mechanism (JDK-8148188) has introduced a regression whereby the System Logger may be invoked too early in the bootstrap phase. This causes issue when JarFile objects are locked by JarFile verifier initialization code. The logging work records an X509 Certificate which is used during the jar file verification/initialization phase and hence leads to an early System.Logger call. One thread invokes the initialization of the Logger framework via ServiceLoader and waits to lock a JarFile in use via another thread. Unfortunately that other thread is also waiting for the System Logger to initialize. For now, I think we can avoid the early Logger initialization via use of a ThreadLocal. I've tried reproducing the reported issue through manual and automated tests but to no avail. I've added a new ServiceLoader test which has concurrent threads. One is loading providers and another is initializing JarFile verifiers. Hope it helps improve code coverage for the future. JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/
Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()
some off line comments suggested that I could move the jar initialization checks to the EventHelper class. With that in place, the EventHelper utility class should never initialize the logging framework early during jar initialization. http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/ regards, Sean. On 16/12/2019 14:15, Seán Coffey wrote: The recent crypto event logging mechanism (JDK-8148188) has introduced a regression whereby the System Logger may be invoked too early in the bootstrap phase. This causes issue when JarFile objects are locked by JarFile verifier initialization code. The logging work records an X509 Certificate which is used during the jar file verification/initialization phase and hence leads to an early System.Logger call. One thread invokes the initialization of the Logger framework via ServiceLoader and waits to lock a JarFile in use via another thread. Unfortunately that other thread is also waiting for the System Logger to initialize. For now, I think we can avoid the early Logger initialization via use of a ThreadLocal. I've tried reproducing the reported issue through manual and automated tests but to no avail. I've added a new ServiceLoader test which has concurrent threads. One is loading providers and another is initializing JarFile verifiers. Hope it helps improve code coverage for the future. JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/
