Code change looks fine to me. I re-look at every place where legacyStrings and prngAlgorithms are used and they are all synchronized. Last time I thought some were not. Sorry.
Only one comment: I like the isProviderInfo() name better, but I notice it was the old name pre-7092821. Thanks, Max > On Jun 9, 2020, at 6:31 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > Webrev updated: http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/ > > Besides addressing Max's comments, I also made updateSecureRandomEntries(...) > method private and removed the synchronized keyword as all of its accesses > are synchronized. > > Thanks, > Valerie > On 6/8/2020 2:33 PM, Valerie Peng wrote: >> Hi Max, >> >> Please find comments in line. >> >> On 6/8/2020 2:34 AM, Weijun Wang wrote: >>> Looks like this should work, but still find it complicated. >>> >>> 1. Do we need to care about thread safety when managing legacyStrings? >> >> Right, it's more complicated than I like as well. >> >> As for thread safety, the legacy relevant data are all synchronized under >> the current provider object, i.e. this. Is there a particular call path not >> doing this? This is the same as the pre-7092821 code. >> >>> 2. Does implReplaceAll() need to set legacyChanged = true? >> Correct, the removal is by accident. Thanks for catching this. >>> 3. How about using prngAlgorithms.iterator().next() below? >>> >>> 1416 return prngAlgorithms.toArray(new String[0])[0]; >> >> Sure, changed. >> >> Valerie >> >>> >>> --Max >>> >>> >>>> On Jun 6, 2020, at 11:54 AM, Valerie Peng <valerie.p...@oracle.com> wrote: >>>> >>>> Thanks for reviewing and sharing the feedbacks on webrev.00. >>>> >>>> In order to support all existing calls for legacy registration for default >>>> secure random, I have to revert some of the JDK-7092821 changes and >>>> re-introduce the 'legacyStrings' LinkedHashMap. Updated the regression >>>> test with removal test for provider using legacy registrations as well. >>>> Although removal is supported, this is still not bullet proof as things >>>> may not work as expected if a provider registered their impl in both ways, >>>> i.e. legacy String pair and Service, and then remove/replace some entries >>>> later. Please comment if you really need this scenario to be supported. >>>> Although not explicitly documented, I think the intention is to use one or >>>> the other, never both. >>>> >>>> Webrev update: http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/ >>>> >>>> Thanks, >>>> Valerie >>>> On 6/5/2020 11:00 AM, Valerie Peng wrote: >>>>> Right, I try to keep the impl simple as I am not aware of any property >>>>> removal usage. >>>>> >>>>> Oh-well, if we have to cater to the removal scenario, then we'd have to >>>>> add a List and keep track of ALL secure random algos instead of only the >>>>> FIRST one. Alright, I guess it costs for covering all aspect. But one >>>>> extra benefit of this is that it should be easy to handle the future JDK >>>>> property such as "jdk.securerandom.disabledAlgorithms" if it were to be >>>>> added. >>>>> >>>>> Valerie >>>>> >>>>> On 6/5/2020 7:54 AM, Weijun Wang wrote: >>>>>> I don't know who in this world would want to do that, but Prasad's >>>>>> concern is technically possible. I tried 'p.remove("SecureRandom.a")' in >>>>>> the new test, and new SecureRandom() fails with >>>>>> "java.security.NoSuchAlgorithmException: a SecureRandom not available". >>>>>> >>>>>> And people can also remove one entry and add it back in order to move it >>>>>> to the end. One can even add new implementations this way. >>>>>> >>>>>> Unfortunately there is no ConcurrentLinkedHashMap. >>>>>> >>>>>> --Max >>>>>> >>>>>>> On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula >>>>>>> <prasadarao.kopp...@oracle.com> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Looks good to me, one question >>>>>>> >>>>>>> If first registered SecureRandom algo gets removed, >>>>>>> getDefaultSecureRandomAlgorithm return stale data, a refresh required >>>>>>> in remove? >>>>>>> >>>>>>> Thanks, >>>>>>> Prasad.K >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Valerie Peng >>>>>>>> Sent: Friday, June 5, 2020 2:52 AM >>>>>>>> To: security-dev@openjdk.java.net >>>>>>>> Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom >>>>>>>> algo >>>>>>>> based on registration ordering >>>>>>>> >>>>>>>> Hi, Sean, >>>>>>>> >>>>>>>> Thanks for the review and feedback. Webrev updated: >>>>>>>> http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/ >>>>>>>> >>>>>>>> getTypeAndAlgorithm(...) was not static due to an instance variable >>>>>>>> used by >>>>>>>> debugging output. I have removed it and made both method static. >>>>>>>> >>>>>>>> I will wait for others' review comments also. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Valerie >>>>>>>> On 6/4/2020 2:01 PM, Sean Mullan wrote: >>>>>>>>> On 6/4/20 3:34 PM, Valerie Peng wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> Could someone help reviewing this fix? This change keep tracks of >>>>>>>>>> the >>>>>>>>>> first registered SecureRandom algorithm and returns it upon the >>>>>>>>>> request of SecureRandom class. >>>>>>>>> This looks good to me. I would recommend that Max or someone else >>>>>>>>> review it as well. >>>>>>>>> >>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246613 >>>>>>>>>> >>>>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/ >>>>>>>>> A couple of minor comments, feel free to fix or ignore. >>>>>>>>> >>>>>>>>> * SecureRandom.java >>>>>>>>> >>>>>>>>> 879 // For SUN provider, we use >>>>>>>>> SunEntries.DEFF_SECURE_RANDOM_ALGO >>>>>>>>> >>>>>>>>> Might as well fix the typo while you are in there: s/DEFF/DEF/ >>>>>>>>> >>>>>>>>> * Provider.java >>>>>>>>> >>>>>>>>> 1156 private String parseSecureRandomPut(String name, String >>>>>>>>> value) { >>>>>>>>> >>>>>>>>> Could be static if you also make getTypeAndAlgorithm static, I think. >>>>>>>>> >>>>>>>>> --Sean
