Looks like this should work, but still find it complicated. 1. Do we need to care about thread safety when managing legacyStrings?
2. Does implReplaceAll() need to set legacyChanged = true? 3. How about using prngAlgorithms.iterator().next() below? 1416 return prngAlgorithms.toArray(new String[0])[0]; --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