[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-security-dev
Subject:    Re: RFR 7004967: SecureRandom should be more explicit about threading
From:       Sean Mullan <sean.mullan () oracle ! com>
Date:       2016-11-17 12:34:10
Message-ID: a4b088d1-9d26-af87-f174-2b9681adda2d () oracle ! com
[Download RAW message or body]

Looks good.

--Sean

On 11/15/16 9:55 PM, Wang Weijun wrote:
> Please review the updated webrev at
>
>    http://cr.openjdk.java.net/~weijun/7004967/webrev.02/
>
> Only spec change [1].
>
> This change also covers 8169312.
>
> Thanks
> Max
>
> [1]
> http://cr.openjdk.java.net/~weijun/7004967/webrev.02/interdiff.patch.html
>
> On 11/4/2016 10:54 PM, Sean Mullan wrote:
>> * SecureRandom
>>
>>  131  * If this attribute is not set or is "false", this class will
>> instead
>>  132  * synchronize access to each of the methods of the {@code
>> SecureRandomSpi}
>>  133  * implementation.
>>
>> Not all of the methods are synchronized - engineGetParameters is not,
>> for example. I think to avoid ambiguity, you should list the names of
>> the methods that are synchronized.
>>
>> 810      * @throws IllegalArgumentException if {@code numBytes} is
>> negative
>>
>> Since this is a new @throws, you really need to file a new bug.
>>
>> Please also file a docs bug with a description of the new attribute.
>>
>> * SecureRandomSpi
>>
>> lines 63-83, I think the wording could be improved/simplified, how about:
>>
>> See {@link SecureRandom} for additional details on thread safety. By
>> default, a SecureRandomSpi implementation is considered to be not safe
>> for use by multiple concurrent threads and SecureRandom will synchronize
>> access to each of the applicable engine methods (see SecureRandom for
>> the list of methods). However, if a SecureRandomSpi implementation is
>> thread-safe, the <a
>> href="{@docRoot}/../technotes/guides/security/StandardNames.html#Service">service
>>
>> provider attribute</a> "ThreadSafe" should be set to "true" during its
>> registration, as follows:
>>
>>   put("SecureRandom.AlgName ThreadSafe", "true");
>>
>>   or
>>
>>   putService(new Service(this, "SecureRandom", "AlgName", className,
>>              null, Map.of("ThreadSafe", "true")));
>>
>> {@code SecureRandom} will then call the applicable engine methods
>> without any synchronization.
>>
>> --Sean
>>
>> On 11/2/16 3:27 AM, Wang Weijun wrote:
>>> Ping again.
>>>
>>> There is an updated version at
>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only
>>> changes.
>>>
>>> Thanks
>>> Max
>>>
>>>> On Aug 25, 2016, at 10:00 AM, Weijun Wang <weijun.wang@oracle.com>
>>>> wrote:
>>>>
>>>> Please review the enhancement at
>>>>
>>>>  http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
>>>>
>>>> Basically, we want SecureRandom to be more efficient by removing all
>>>> synchronized keywords from its public methods and let an
>>>> implementation to take care of thread-safety (We already did some in
>>>> JDK-8098581). On the other hand, we need to make sure that existing
>>>> implementations that have not synchronized correctly to behave just
>>>> as good as before.
>>>>
>>>> Therefore a new Service Attribute "ThreadSafe" is introduced. If you
>>>> think your implementation is already thread-safe, set it to "true"
>>>> and SecureRandom will be happy. Otherwise, don't set it and
>>>> SecureRandom will continuously call your SecureRandomSpi engine
>>>> methods in synchronized blocks.
>>>>
>>>> Thanks
>>>> Max
>>>
[prev in list] [next in list] [prev in thread] [next in thread]