Re: RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
No problem, thanks for double checking. ;)
Valerie
On 3/12/2020 4:42 AM, Daniel Fuchs wrote:
Hi Valerie,
Please ignore my comment. Sorry for the noise.
I somehow clicked on the wrong webrev link :-(
best regards,
-- daniel
On 12/03/2020 11:35, Daniel Fuchs wrote:
Hi Valerie,
Given that hasKeyAttributes is already decelared as volatile,
may I suggest the following change that uses double locking?
It will avoid synchronizing in the happy case where `hasKeyAttributes`
has already been computed.
1924 private boolean hasKeyAttributes() {
1925 Boolean b = hasKeyAttributes;
if (b != null) return b;
synchronized (this) {
b = hasKeyAttributes;
1926 if (b != null) return b;
1927 String s;
1928 s = getAttribute("SupportedKeyFormats");
...
1948 hasKeyAttributes = b;
1949 }
1950 return b;
1951 }
best regards,
-- daniel
On 11/03/2020 20:31, Valerie Peng wrote:
Anyone can help reviewing this?
I addressed this by applying the double-checked-locking pattern for
lazy initialization of instance fields.
Existing code already have most of the code structured for the
pattern but misses the second check.
Bug: https://bugs.openjdk.java.net/browse/JDK-8238566
Webrev: http://cr.openjdk.java.net/~valeriep/8238566/webrev.00/
Thanks,
Valerie
Re: RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
Hi Valerie,
Please ignore my comment. Sorry for the noise.
I somehow clicked on the wrong webrev link :-(
best regards,
-- daniel
On 12/03/2020 11:35, Daniel Fuchs wrote:
Hi Valerie,
Given that hasKeyAttributes is already decelared as volatile,
may I suggest the following change that uses double locking?
It will avoid synchronizing in the happy case where `hasKeyAttributes`
has already been computed.
1924 private boolean hasKeyAttributes() {
1925 Boolean b = hasKeyAttributes;
if (b != null) return b;
synchronized (this) {
b = hasKeyAttributes;
1926 if (b != null) return b;
1927 String s;
1928 s = getAttribute("SupportedKeyFormats");
...
1948 hasKeyAttributes = b;
1949 }
1950 return b;
1951 }
best regards,
-- daniel
On 11/03/2020 20:31, Valerie Peng wrote:
Anyone can help reviewing this?
I addressed this by applying the double-checked-locking pattern for
lazy initialization of instance fields.
Existing code already have most of the code structured for the pattern
but misses the second check.
Bug: https://bugs.openjdk.java.net/browse/JDK-8238566
Webrev: http://cr.openjdk.java.net/~valeriep/8238566/webrev.00/
Thanks,
Valerie
Re: RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
Hi Valerie,
Given that hasKeyAttributes is already decelared as volatile,
may I suggest the following change that uses double locking?
It will avoid synchronizing in the happy case where `hasKeyAttributes`
has already been computed.
1924 private boolean hasKeyAttributes() {
1925 Boolean b = hasKeyAttributes;
if (b != null) return b;
synchronized (this) {
b = hasKeyAttributes;
1926 if (b != null) return b;
1927 String s;
1928 s = getAttribute("SupportedKeyFormats");
...
1948 hasKeyAttributes = b;
1949 }
1950 return b;
1951 }
best regards,
-- daniel
On 11/03/2020 20:31, Valerie Peng wrote:
Anyone can help reviewing this?
I addressed this by applying the double-checked-locking pattern for lazy
initialization of instance fields.
Existing code already have most of the code structured for the pattern
but misses the second check.
Bug: https://bugs.openjdk.java.net/browse/JDK-8238566
Webrev: http://cr.openjdk.java.net/~valeriep/8238566/webrev.00/
Thanks,
Valerie
Re: RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
Looks fine to me. Xuelei On 3/11/2020 1:31 PM, Valerie Peng wrote: Anyone can help reviewing this? I addressed this by applying the double-checked-locking pattern for lazy initialization of instance fields. Existing code already have most of the code structured for the pattern but misses the second check. Bug: https://bugs.openjdk.java.net/browse/JDK-8238566 Webrev: http://cr.openjdk.java.net/~valeriep/8238566/webrev.00/ Thanks, Valerie On 2/5/2020 12:49 PM, Arthur Eubanks wrote: Webrev: http://cr.openjdk.java.net/~aeubanks/8238566/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8238566 Found by TSAN, java.security.Provider$Service.supportsParameter() is racy. We haven't observed any actual bad behavior, but reasoning through it seems like bad behavior is possible. http://hg.openjdk.java.net/jdk/jdk/file/62b5bfef8d61/src/java.base/share/classes/java/security/Provider.java#l1927 The synchronized block seems to not have any effect on correctness. Example race: T1 in hasKeyAttributes() writes true to hasKeyAttributes and fills out supportedFormats/supportedClasses. T2 in hasKeyAttributes() racily reads hasKeyAttributes as true, but then in supportsKeyFormat() racily reads supportedFormats as null. It can then improperly return false from supportsParameter(). There is no synchronization between T1 and T2 because T2 never does any synchronization, so T2 can read what T1 writes in any order. Fix is to make all of hasKeyAttributes() synchronized.
Re: RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
Anyone can help reviewing this? I addressed this by applying the double-checked-locking pattern for lazy initialization of instance fields. Existing code already have most of the code structured for the pattern but misses the second check. Bug: https://bugs.openjdk.java.net/browse/JDK-8238566 Webrev: http://cr.openjdk.java.net/~valeriep/8238566/webrev.00/ Thanks, Valerie On 2/5/2020 12:49 PM, Arthur Eubanks wrote: Webrev: http://cr.openjdk.java.net/~aeubanks/8238566/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8238566 Found by TSAN, java.security.Provider$Service.supportsParameter() is racy. We haven't observed any actual bad behavior, but reasoning through it seems like bad behavior is possible. http://hg.openjdk.java.net/jdk/jdk/file/62b5bfef8d61/src/java.base/share/classes/java/security/Provider.java#l1927 The synchronized block seems to not have any effect on correctness. Example race: T1 in hasKeyAttributes() writes true to hasKeyAttributes and fills out supportedFormats/supportedClasses. T2 in hasKeyAttributes() racily reads hasKeyAttributes as true, but then in supportsKeyFormat() racily reads supportedFormats as null. It can then improperly return false from supportsParameter(). There is no synchronization between T1 and T2 because T2 never does any synchronization, so T2 can read what T1 writes in any order. Fix is to make all of hasKeyAttributes() synchronized.
RFR: 8238566: java.security.Provider$Service.supportsParameter() is racy
Webrev: http://cr.openjdk.java.net/~aeubanks/8238566/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8238566 Found by TSAN, java.security.Provider$Service.supportsParameter() is racy. We haven't observed any actual bad behavior, but reasoning through it seems like bad behavior is possible. http://hg.openjdk.java.net/jdk/jdk/file/62b5bfef8d61/src/java.base/share/classes/java/security/Provider.java#l1927 The synchronized block seems to not have any effect on correctness. Example race: T1 in hasKeyAttributes() writes true to hasKeyAttributes and fills out supportedFormats/supportedClasses. T2 in hasKeyAttributes() racily reads hasKeyAttributes as true, but then in supportsKeyFormat() racily reads supportedFormats as null. It can then improperly return false from supportsParameter(). There is no synchronization between T1 and T2 because T2 never does any synchronization, so T2 can read what T1 writes in any order. Fix is to make all of hasKeyAttributes() synchronized.
