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

2016-09-20 Thread 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: [9]RFR 8136355: CKM_SSL3_KEY_AND_MAC_DERIVE no longer available by default on Solaris 12

2016-09-20 Thread Xuelei Fan 

P11TlsKeyMaterialGenerator.java
102-106:
There is a bug in the previous code. "&&" should be replaced with "||".
-   (version < 0x0300) && (version > 0x0302)
+   (version < 0x0300) || (version > 0x0302)

The other two have the same issues.  Otherwise, looks fine to me.

BTW, if client request to negotiate SSLv3, the server may not be able to 
select other crypto provider that supports SSLv3 at present.  We may 
want a further enhancement later.  As SSLv3 is fading out, this 
enhancement may be not our priority.  I filed a P3 RFE (JDK-8166425) for 
the tracking.


Xuelei


On 9/20/2016 8:31 AM, Valerie Peng wrote:

Xuelei,

Could you please help reviewing this change?

There are quite a few test failures on Solaris 12 due to the removal of
Solaris PKCS11 SSL3 mechanisms which SunPKCS11 provider assume to be
always present. I updated relevant classes as well as regression tests
to skip SSL3 testing when the support isn't there.

Bug: https://bugs.openjdk.java.net/browse/JDK-8136355
Webrev: http://cr.openjdk.java.net/~valeriep/8136355/webrev.00/

Thanks,
Valerie

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

2016-09-20 Thread 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

2016-09-20 Thread 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

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

2016-09-20 Thread Artem Smotrakov 

Hello,

If you don't mind, I moved some common code from SSLSocketTemplate.java 
to SSLTest.java, so that it can be re-used by other tests. It may help 
to avoid duplicate code.


http://cr.openjdk.java.net/~asmotrak/8164591/webrev.01/

Please take a look.

Artem


On 09/15/2016 11:49 AM, Artem Smotrakov wrote:

Hi Xuelei, Chris,

Thank you for looking into it. Please see inline.


On 09/15/2016 12:53 AM, Chris Hegarty wrote:

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:

On 9/15/2016 9:45 AM, Artem Smotrakov wrote:
Well, in this particular case it's not clear that it has the same 
issue
with free port (at least to me). The exception occurred on client 
side,
so it's not the case where we don't know where the handshake came 
from.


;-) Yeh, you catch the point.  But there is a free-port issue 
although the exception stack in the bug description may be not that 
case.


Let's look at a scenarios:
1. server open a server socket and listen.
2. other test case connect to the server socket.
3. this test case try to connect to server socket.
4. this test case would fail as the server only accept one connections.

I did not check it very carefully, I think for #4, the exception 
stack can be similar to the one in the bug description.

Looks like a rare, but valid case.


Anyway, as a free port is used, there are free-port issues. Please 
consider to make the enhancement in the fix. Otherwise, you cannot 
avoid the intermittent failure for this test case in the current 
testing environment.

+1.   Please remove any use of the free-port anti-pattern.
Just to be clear, this is not an issue with getting a free port with 
ServerSocket.getLocalPort() and them re-using it to create a new 
ServerSocket. It's more tricky (for example, please see the scenario 
above).


Okay, let me update it to follow the approach which is implemented in 
SSLSocketSample.java


Artem


-Chris.


Xuelei

I can make this enhancement, but like I said I don't think it's 
going to

help, so I would like to keep debug output on.

Artem


On 09/14/2016 06:39 PM, Xuelei Fan wrote:

On 9/15/2016 9:23 AM, Artem Smotrakov wrote:

Hi Xuelei,

For this one, I am not sure that it would help here since the test
failed after it already had started handshaking.

It has the same issue as a free-port is used.  We don't actually know
the handshake is coming from the right client.

Xuelei


I would prefer to have it as a separate enhancement.

Artem


On 09/14/2016 06:19 PM, Xuelei Fan wrote:

As you were already there, I would suggest to consider the
SSLSocketSample.java template as the comment in JDK-8163924 review
thread.

Thanks,
Xuelei

On 9/15/2016 9:13 AM, Artem Smotrakov wrote:
Not urgent, but I would appreciate if someone can get a chance 
to look

at this.

Artem


On 09/07/2016 03:17 PM, Artem Smotrakov wrote:

Sending to net-...@openjdk.java.net as well.

Artem


On 09/07/2016 12:28 PM, Artem Smotrakov wrote:

Hello,

Please review the following patch for
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java

The test has been observed to fail a couple of times, but 
it's still
not clear why it failed because there is not much info in 
logs. The
patch updates the test to enable additional debug output, so 
that we

have more info if it fails next time.

While looking at the test, I notices a couple of issues, but 
they

don't seem to cause these intermittent failures:
- The test sets system properties for JSSE in a loop, but JSSE
provider reads them only once while initialization. As a result,
only
values which were set in the first iteration are actually used.
- The test doesn't close files and sockets sometimes.

The patch also fixed the issues above, and there are a couple
cosmetic changes.

Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/

Artem

RFR(s): 8166378: Missing dependencies in several java/security tests

2016-09-20 Thread Sergei Kovalev 

Hello team,

Please review very small fix for several regression tests.

BugID: https://bugs.openjdk.java.net/browse/JDK-8166378
WebReview: http://cr.openjdk.java.net/~skovalev/8166378/webrev.00

Issue: The tests has no declared dependencies on jdk.security modules. 
This leads the tests to fail in case of usage "--limit-modules" java 
command line option.


Solution: add missed dependencies if required.

--
With best regards,
Sergei