Re: OpenJDK11u: Backward incompatible behavior
Hello Xuelei, Thank you for review. Can I commit it to JDK15 and create backports to JDK 14, 13 and 11 ? Thank you Alexey > On 10 Mar 2020, at 20:59, Xuelei Fan wrote: > > Looks fine to me. > > Thanks, > Xuelei > > On 3/5/2020 8:50 AM, Alexey Bakhtin wrote: >> Hello Xuelei, >> I have renamed inputBuffer to recordBody. >> Also, as you suggested, recordBody is not removed but used for multiple >> records. So, it should be better for performance. >> JDK15 webrev: http://cr.openjdk.java.net/~dcherepanov/8239788/webrev.v5/ >> Regards >> Alexey >>> On 4 Mar 2020, at 21:23, Xuelei Fan wrote: >>> http://cr.openjdk.java.net/~bae/8239788/webrev.v4/ >>> >>> SSLSocketInputRecord: >>> 54 // Cache for incomplete input record. >>> 55 private ByteBuffer inputBuffer = null; >>> This variable is used for record body, I may use a instinctive name, for >>> example recordBody. >>> >>> Otherwise, looks good to me. >>> >>> I think, for performance, it may be possible to reuse this buffer for >>> multiple records. I'd appreciate if you want to make an improvement in >>> this update as well. >>> >>> Thanks, >>> Xuelei >>> signature.asc Description: Message signed with OpenPGP
Re: OpenJDK11u: Backward incompatible behavior
Hi Alexey, I had run the testing for you, no surprise. Please commit to JDK 15, and backport accordingly. Thanks, Xuelei On 3/11/2020 7:16 AM, Alexey Bakhtin wrote: Hello Xuelei, Thank you for review. Can I commit it to JDK15 and create backports to JDK 14, 13 and 11 ? Thank you Alexey On 10 Mar 2020, at 20:59, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 3/5/2020 8:50 AM, Alexey Bakhtin wrote: Hello Xuelei, I have renamed inputBuffer to recordBody. Also, as you suggested, recordBody is not removed but used for multiple records. So, it should be better for performance. JDK15 webrev: http://cr.openjdk.java.net/~dcherepanov/8239788/webrev.v5/ Regards Alexey On 4 Mar 2020, at 21:23, Xuelei Fan wrote: http://cr.openjdk.java.net/~bae/8239788/webrev.v4/ SSLSocketInputRecord: 54 // Cache for incomplete input record. 55 private ByteBuffer inputBuffer = null; This variable is used for record body, I may use a instinctive name, for example recordBody. Otherwise, looks good to me. I think, for performance, it may be possible to reuse this buffer for multiple records. I'd appreciate if you want to make an improvement in this update as well. Thanks, Xuelei
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.
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: 8237219: Disabling the native SunEC implementation
Another webrev update with Max's recent comments. https://cr.openjdk.java.net/~ascarpino/8237219/webrev.03 Also I still need a reviewer for the CSR. thanks Tony On 3/2/20 4:40 PM, Anthony Scarpino wrote: Hi I need a review of the CSR and webrev for disabling by default the native SunEC curves from the API. With the recent verification changes in JDK-8237218, SunJCE is long dependent on the native code for verifying the constant-time curves. This disabling can be undone with setting a system property, jdk.sunec.disableNative. I'm doing a simultaneous review as changes for one will likely affect the other. CSR: https://bugs.openjdk.java.net/browse/JDK-8238911 webrev: https://cr.openjdk.java.net/~ascarpino/8237219/ The curves affected are: secp112r1, secp112r2, secp128r1, secp128r2, secp160k1, secp160r1, secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, sect113r2, sect131r1, sect131r2, sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, sect239k1, sect283k1, sect283r1, sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, X9.62 prime192v2, X9.62 prime192v3, X9.62 prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 Tony
