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

[Xuelei Fan]

Looks fine to me.  Thanks!

Xuelei

On 9/22/2016 4:05 PM, Jamil Nimeh wrote:

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) & 0x7FFF;  // 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 0x1FFF, 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 & 0x7FFF) % 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) & 0x7FFF;  // 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 
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

Re: 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) & 0x7FFF;  // 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 0x1FFF, 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 & 0x7FFF) % 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) & 0x7FFF;  // 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 
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

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

[Xuelei Fan]

> latch = (latch + 1) & 0x7FFF;  // 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 0x1FFF, 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 & 0x7FFF) % 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) & 0x7FFF;  // 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 
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

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

[Jamil Nimeh]

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 & 0x7FFF) % 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) & 0x7FFF;  // 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  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

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

[Wang Weijun]

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  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

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

[Jamil Nimeh]

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


On 09/20/2016 10:32 PM, Jamil Nimeh wrote:

Hi Max and Xuelei, thanks for the feedback.


On 09/20/2016 07:52 PM, Wang Weijun wrote:

On Sep 21, 2016, at 9:58 AM, Xuelei Fan  wrote:

  359   while (System.nanoTime() - startTime < 25000) {
  360   synchronized(this){};
- 361   latch++;
+ 361   latch = (latch + 1) % Integer.MAX_VALUE;
  362   }

This block may be not CPU friendly as it may loop a large amount of 
times in a very short period (250milli).
You asked about the empty synchronized block also: From what I've been 
reading on this topic it looks like the use of the empty synchronized 
block can be used to force cache coherency between multiple threads.  
In terms of it being CPU intensive, has seed generation ever pegged a 
processor in the past?


There were cases in the past where it would hang, but that was fixed 
back when Max changed things in the inner loop to use 
System.nanoTime()  (see JDK-8157318) but at that point (only 3 months 
ago) we didn't feel the need to restructure the loop.  I don't know 
that we do at this point either.  But we certainly can fix the 
overflow of the latch easily enough.

To get a <255 index I think we only need to loop for <66536 times.

How about we stop at every millisecond and see if it's enough? 
Something like this:


 long next = startTime + 100;
 while (next < startTime + 25000) {
 while (System.nanoTime() < next) {
 synchronized(this){};
 latch++;
 }
 if (latch > 65535 || latch < 0) break;
 next += 100;
 }


What's the usage of line 360?  Just for some computation?

367   counter += latch;
The counter variable may be overflow too.
I find this strange. Were computers so slow in 1996 that within 250ms 
latch cannot exceed 64000?
1996?  You're talking about pentium and pentium 2 machines so at best 
you're talking 450MHz.  I don't know if the latch wouldn't pop under 
those conditions.


As for the counter, a potential overflow I don't think is that bad 
given the way the loop control is written.  At worst it just means 
another spin around the outer loop and another byte dropped in the 
pool.  And that loop can only iterate 6 times at the most so it's not 
like things can run away.





--Max


Xuelei

On 9/21/2016 8:57 AM, Jamil Nimeh wrote:

Hello all,

This fixes a bug found in stress testing where on faster CPUs the 
latch

can overflow resulting in a negative array index.  The fix avoids the
overflow by resetting the latch to 0 when it reaches 
Integer.MAX_VALUE -

1 and will continue increasing from there.

Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/

Thanks,
--Jamil

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

[Jamil Nimeh]

Hi Max and Xuelei, thanks for the feedback.


On 09/20/2016 07:52 PM, Wang Weijun wrote:

On Sep 21, 2016, at 9:58 AM, Xuelei Fan  wrote:

  359   while (System.nanoTime() - startTime < 25000) {
  360   synchronized(this){};
- 361   latch++;
+ 361   latch = (latch + 1) % Integer.MAX_VALUE;
  362   }

This block may be not CPU friendly as it may loop a large amount of times in a 
very short period (250milli).
You asked about the empty synchronized block also: From what I've been 
reading on this topic it looks like the use of the empty synchronized 
block can be used to force cache coherency between multiple threads.  In 
terms of it being CPU intensive, has seed generation ever pegged a 
processor in the past?


There were cases in the past where it would hang, but that was fixed 
back when Max changed things in the inner loop to use System.nanoTime()  
(see JDK-8157318) but at that point (only 3 months ago) we didn't feel 
the need to restructure the loop.  I don't know that we do at this point 
either.  But we certainly can fix the overflow of the latch easily enough.

To get a <255 index I think we only need to loop for <66536 times.

How about we stop at every millisecond and see if it's enough? Something like 
this:

 long next = startTime + 100;
 while (next < startTime + 25000) {
 while (System.nanoTime() < next) {
 synchronized(this){};
 latch++;
 }
 if (latch > 65535 || latch < 0) break;
 next += 100;
 }


What's the usage of line 360?  Just for some computation?

367   counter += latch;
The counter variable may be overflow too.

I find this strange. Were computers so slow in 1996 that within 250ms latch 
cannot exceed 64000?
1996?  You're talking about pentium and pentium 2 machines so at best 
you're talking 450MHz.  I don't know if the latch wouldn't pop under 
those conditions.


As for the counter, a potential overflow I don't think is that bad given 
the way the loop control is written.  At worst it just means another 
spin around the outer loop and another byte dropped in the pool.  And 
that loop can only iterate 6 times at the most so it's not like things 
can run away.





--Max


Xuelei

On 9/21/2016 8:57 AM, Jamil Nimeh wrote:

Hello all,

This fixes a bug found in stress testing where on faster CPUs the latch
can overflow resulting in a negative array index.  The fix avoids the
overflow by resetting the latch to 0 when it reaches Integer.MAX_VALUE -
1 and will continue increasing from there.

Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/

Thanks,
--Jamil

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

[Wang Weijun]

> On Sep 21, 2016, at 9:58 AM, Xuelei Fan  wrote:
> 
>  359   while (System.nanoTime() - startTime < 25000) {
>  360   synchronized(this){};
> - 361   latch++;
> + 361   latch = (latch + 1) % Integer.MAX_VALUE;
>  362   }
> 
> This block may be not CPU friendly as it may loop a large amount of times in 
> a very short period (250milli).

To get a <255 index I think we only need to loop for <66536 times.

How about we stop at every millisecond and see if it's enough? Something like 
this:

long next = startTime + 100;
while (next < startTime + 25000) {
while (System.nanoTime() < next) {
synchronized(this){};
latch++;
}
if (latch > 65535 || latch < 0) break;
next += 100;
}

> 
> What's the usage of line 360?  Just for some computation?
> 
> 367   counter += latch;
> The counter variable may be overflow too.

I find this strange. Were computers so slow in 1996 that within 250ms latch 
cannot exceed 64000?

--Max

> 
> Xuelei
> 
> On 9/21/2016 8:57 AM, Jamil Nimeh wrote:
>> Hello all,
>> 
>> This fixes a bug found in stress testing where on faster CPUs the latch
>> can overflow resulting in a negative array index.  The fix avoids the
>> overflow by resetting the latch to 0 when it reaches Integer.MAX_VALUE -
>> 1 and will continue increasing from there.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/
>> 
>> Thanks,
>> --Jamil

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

[Jamil Nimeh]

Hello all,

This fixes a bug found in stress testing where on faster CPUs the latch 
can overflow resulting in a negative array index.  The fix avoids the 
overflow by resetting the latch to 0 when it reaches Integer.MAX_VALUE - 
1 and will continue increasing from there.


Bug: https://bugs.openjdk.java.net/browse/JDK-8049516
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8049516/webrev.01/

Thanks,
--Jamil