> On Apr 20, 2016, at 11:34 AM, Xuelei Fan <xuelei....@oracle.com> wrote: > > On 4/19/2016 9:09 PM, Xuelei Fan wrote: >> On 4/15/2016 9: >>> http://cr.openjdk.java.net/~weijun/8051408/webrev.10/ > > src/java.base/share/classes/sun/security/provider/AbstractDrbg.java > =================================================================== > line 66-68: My understanding is that ... > > I would suggest rewords or remove this sentence. "Not used much" does > not mean needing no synchronization.
Of course. Precisely engineNextBytes() should synchronize on both states and configuration, and engineGenerateSeed() should synchronize only on configuration. But since engineGenerateSeed() is not used a lot, I don't think it's not worth coding it with a special synchronize(configuration) so both now just synchronize on "this". > As you have add synchronized > keyword for engineGenerateSeed, I may suggest you remove lines 63-68, > and move 57-61 to class description. I'll move 57-61 to class description, and would like to keep 63-68 there, and update the words "does not need to be synchronized" to "does not need to be synchronized on the internal states". > > I think comment are mainly used for code readers. You used a lot "my" > and "I", and questions to yourself in the comments. Would you mind > change to use neutral tones and remove unnecessary comments in the final > code? OK. > > ------ > 662 // 6/7. Get entropy. TODO: 1st arg security strength? > > Do you want to complete the TODO task. Similar comments to other TODO > tasks. Will remove TODO word and add a clarification. > > ------ > 670 nonce = longToByteArray(cc.incrementAndGet()); > 685 private static byte[] longToByteArray(long l) > > The nonce is leading with "sun.drbg", and following by a 8-bytes integer > value. This scheme only provider 8-bytes randomness. Looks like the > quality does not meet the nonce requirements (8.7.6 NIST SP-800-90Ar1) > for 256-bit strength. 8.6.7 allows it to be a "monotonically increased sequence number". > > ------ > 598 protected final synchronized void engineConfigure( > > engineConfigure is not part of SPI. Would you mind change the name and > remove the leading "engine" in case of miss-understanding? s/engineConfigure/configure/ > > It would be nice grant as less accessibility as possible. For the > "protected" keyword, should it be package accessibility at present? > Similar comment for other protected methods other than the override > ones, and some public classes in the package. Will go through them. I created AbstractDrbg so that it can be the base for any DRBG not necessarily in this package. For example, the AbstractDrbgSpec test also contains an implementation. > > ------ > Section 7.2 of NIST SP 800-90Ar1 says: "The personalization string > should be unique for all instantiations of the same DRBG mechanism type". > > Please check the unique for the personalization string in the > implementation. "Should" is not "shall" (section 4, terms). Two other reasons: 1. Checking for uniqueness needs to save all strings in memory. 2. The default is null, and all nulls are the same. Why bother checking for those non-nulls for uniqueness? Thanks Max > > Looking forward to your next webrev. > > Xuelei > >
