RFR: JDK-8049516: sun.security.provider.SeedGenerator throws ArrayIndexOutOfBoundsException

[Jamil Nimeh]

That's a very good suggestion.  I've made that change and updated the 
webrev:

http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.02/

Thanks again,
--Jamil

On 09/21/2016 05:34 PM, Xuelei Fan wrote:
> > latch = (latch + 1) & 0x7FFFFFFF;  // Mask the sign bit
> I'm fine with it.
>
> BTW, if you like, I may suggest to use a little bit small number for 
> the mask, for example 0x1FFFFFFF, so that the counter variable does 
> not overflow either.  It works if counter overflow, but better not if 
> we want the counter meaningful.
>
> Xuelei
>
>
> On 9/22/2016 7:29 AM, Jamil Nimeh wrote:
>> So here are a couple ideas related to your suggestion.
>>
>> We can leave a simple increment on latch and let it overflow, then in
>> the array access do this:
>>
>> v ^= rndTab[(latch & 0x7FFFFFFF) % 255];
>>
>> That clears the high order bit and the mod will keep it in the range of
>> 0-254.  the lvalue for the mod operation will never go negative.  The
>> downside of that is counter will still get assigned a large negative
>> value (which as I said earlier could cause another iteration of the main
>> loop...not the end of the world since it can only happen 6 times max).
>>
>> The other approach would be to increment latch and mask off the high
>> order bit:
>>
>> latch = (latch + 1) & 0x7FFFFFFF;  // Mask the sign bit
>>
>> Then the v^=[latch % 255] is safe as-is and counter doesn't get some
>> massively negative value (though it can still overflow across multiple
>> iterations of the outer loop).
>>
>> I like the second approach a little better, personally. Up-front use of
>> the bitwise-and is a little more clear that we're forcing the value to
>> be non-negative before we use it as an array index input.  Let me know
>> what you think and I'll update the webrev accordingly.
>>
>> Thanks,
>> --Jamil
>>
>> On 09/21/2016 09:05 AM, Wang Weijun wrote:
>>> I am OK with your fix, but I found "(latch + 1) % Integer.MAX_VALUE" a
>>> little difficult to understand. Does rndTab[latch & 0xff] also work?
>>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Sep 21, 2016, at 11:57 PM, Jamil Nimeh <jamil.j.nimeh at oracle.com>
>>>> wrote:
>>>>
>>>> Hi Max and Xuelei,
>>>>
>>>> Yesterday I also reached out to the SQE engineer who submitted the
>>>> bug, asking if this is an issue he's seen going forward from the
>>>> original instance in 8u20.  He said that he hasn't seen this issue
>>>> come up since the original bug submission.  Since the simple overflow
>>>> condition is easily solved with my fix, and the code has been
>>>> otherwise stable I'd like to suggest that we keep the fix as-is.  The
>>>> loop timing as it stands now is not the source of the bug, other than
>>>> that latch can overflow and that is solved in one line.  If we want
>>>> to revisit this and improve the overall performance (though I haven't
>>>> seen evidence that there is a perf issue with this at all) then maybe
>>>> an RFE is in order.  What do you think?
>>>>
>>>> --Jamil
>>