[9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error

Jamil Nimeh jamil.j.nimeh at oracle.com
Fri Aug 12 21:38:08 UTC 2016 

Thank you Artem.  The fix looks good.  You just need a +1 from an official reviewer.


--Jamil
-------- Original message --------From: Artem Smotrakov <artem.smotrakov at oracle.com> Date: 8/12/16  1:07 PM  (GMT-08:00) To: Jamil Nimeh <jamil.j.nimeh at oracle.com>, Security Dev OpenJDK <security-dev at openjdk.java.net> Subject: Re: [9] RFR: 8162484: javax/net/ssl/Stapling/SSLSocketWithStapling.java test fails intermittently with "Address already in use" error 
No problem.

http://cr.openjdk.java.net/~asmotrak/8162484/webrev.02/

Artem


On 08/12/2016 12:02 PM, Jamil Nimeh wrote:
> For the tests as we use them today we don't intend the server to 
> restart.  The intent of SimpleOCSPServer was to be of use for a 
> variety of testing purposes.  I don't know that we can say for all 
> intended uses that we'll *never* need to restart it. That's why I'd 
> like to keep the unbound socket/set sockopt/bind/listen behavior.  I 
> don't think ServerSocket(0) achieves that.
>
> --Jamil
>
> On 8/12/2016 11:30 AM, Artem Smotrakov wrote:
>> Hi Jamil,
>>
>> There was no any specific reason to remove ServerSocket.bind() call. 
>> ServerSocket(0) constructor creates a server socket, automatically 
>> bound to a random free port. If I am not missing something, it 
>> doesn't look necessary to set the SO_REUSEADDR socket options if the 
>> server is not going to restart. The code is just shorter if we use 
>> ServerSocket(0) constructor to open a server socket, but I can revert 
>> it to use bind() with 0 port number if you think it's better.
>>
>> Artem
>>
>>
>> On 08/12/2016 09:13 AM, Jamil Nimeh wrote:
>>> Hi Artem, more comments in-line
>>>
>>>
>>> On 8/11/2016 11:46 AM, Artem Smotrakov wrote:
>>>> Hi Jamil,
>>>>
>>>> Thank you for review. Please see inline.
>>>>
>>>>
>>>> On 08/10/2016 04:16 PM, Jamil Nimeh wrote:
>>>>> Hi Artem,
>>>>>
>>>>> I'm not an official reviewer but the solution for making the 
>>>>> servers reject connections rather than stop and start looks pretty 
>>>>> fair to me and seems like a nice way to simulate a downed OCSP 
>>>>> responder instead of having to bounce it.  A couple 
>>>>> comments/questions:
>>>>>
>>>>> I'm a bit surprised that you get the "Address already in use" 
>>>>> error though. 
>>>> Well, to be honest, I was not able to reproduce this failure 
>>>> locally. I was running the test in a loop for a couple of days, and 
>>>> it didn't fail. But the test has been observed to fail in other 
>>>> test runs (jprt, CI, etc).
>>>>
>>>> I am not an expert in networking, and I would appreciate if someone 
>>>> more knowledgeable gives an advise how these intermittent failures 
>>>> can be avoided.
>>>>
>>>>> Isn't servSocket.setReuseAddress(true) on line 214 supposed to set 
>>>>> the SO_REUSEADDR at the system call level and prevent EADDRINUSE 
>>>>> when listening or binding?
>>>> If I am not missing something, the test has been observed to fails 
>>>> while re-binding. I am wondering if it's possible that the port 
>>>> becomes busy after the server socket was closed, but before bind() 
>>>> is called again. The probability of this situation seems to be very 
>>>> low which has been actually seen - the test fails very rare.
>>>>
>>>> If this is the case, it seems like servSocket.setReuseAddress(true) 
>>>> doesn't help because the port is taken by another process (I am not 
>>>> sure that SO_REUSEADDR prevents from this). Again, this is only my 
>>>> guess, and I may be wrong.
>>> You know, I hadn't thought of that.  I've never been able to 
>>> reproduce that problem either, but I'm running on a local virtual 
>>> box VM on a laptop, and usually the tests are running sequentially. 
>>> I could definitely see the case where other processes are soaking up 
>>> the OCSP responder's port.  With those tests, I kind of need the 
>>> port to remain the same since I'm putting that server and port in 
>>> the AIA extensions of the certs for which it answers.  Given this 
>>> particular case, it seems like your solution of keeping the server 
>>> bound but just chopping connections off is the best way to go.
>>>>>
>>>>> When you create the new ServerSocket on line 212, you're now 
>>>>> binding it to the port now where originally it started as an 
>>>>> unbound socket.  By doing so, the behavior of setReuseAddress() on 
>>>>> line 214 is now undefined.
>>>> This setReuseAddress() call looks unnecessary now. I'll update the 
>>>> test.
>>>>> While this test no longer stops/starts the server, other tests may 
>>>>> wish to do so and their behavior may not be consistent (though 
>>>>> apparently it wasn't consistent even in the old scheme where the 
>>>>> socket was unbound, then setReuseAddress() was called...)
>>>> Correct. I checked other code which depend on SimpleOCSPServer
>>>>
>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java
>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java
>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java
>>>> javax/net/ssl/Stapling/StapleEnableProps.java
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 
>>>>
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 
>>>>
>>>> artem at artem-laptop:~/ws/jdk/jdk9_dev_stapling_test/jdk/test$ kate 
>>>> javax/net/ssl/Stapling/HttpsUrlConnClient.java 
>>>> javax/net/ssl/Stapling/SSLEngineWithStapling.java 
>>>> javax/net/ssl/Stapling/SSLSocketWithStapling.java 
>>>> javax/net/ssl/Stapling/StapleEnableProps.java 
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusReqSelection.java 
>>>> sun/security/ssl/StatusStapling/java.base/sun/security/ssl/StatusResponseManagerTests.java 
>>>>
>>>>
>>>> These tests call stop() only once when actual testcases are done. 
>>>> Actually, some of them don't even call stop(), but it seems to work 
>>>> fine. As an enhancement, I would add stop() calls to finally 
>>>> blocks, but it seems to work fine without it anyway.
>>> I liked your solution with the stop() calls in finally blocks and I 
>>> agree that they should have them.  I think we get away with it 
>>> because in most if not all of those cases they are running as 
>>> othervm tests (because we have properties that we set specific to 
>>> the tests).  So when the JVM exits resources like sockets are closed 
>>> by the OS.  Still, it's better to have the try/finally guards and 
>>> explicitly and gracefully shutdown the OCSP responders.
>>>>
>>>> Here is an updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~asmotrak/8162484/webrev.01/
>>> I realize that in many of these test cases we're going to move away 
>>> from a start/stop approach to your accept/reject one, but in general 
>>> sockets designed to be listening should start unbound, set the 
>>> SO_REUSEADDR sockopt, then bind and listen. Was there a specific 
>>> reason to change that code, or was it just to streamline it?  Aside 
>>> from fewer lines of code, I'm not sure what it buys us.
>>>>
>>>> Artem
>>>>
>>>>
>>>>>
>>>>> --Jamil
>>>>>
>>>>>
>>>>> On 08/10/2016 03:44 PM, Artem Smotrakov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review this update for OCSP stapling tests.
>>>>>>
>>>>>> The tests use 
>>>>>> test/java/security/testlibrary/SimpleOCSPServer.java which try to 
>>>>>> re-use a server port if the server restarted. Looks like 
>>>>>> sometimes it may cause "Address already in use" error.
>>>>>>
>>>>>> The patch updates OCSP stapling tests with the following:
>>>>>> - updated SSLSocketWithStapling.java test not to restart OCSP 
>>>>>> responders
>>>>>> - updated SimpleOCSPServer to be able to reject incoming connections
>>>>>> - updated SimpleOCSPServer to be able to reproduce a delay 
>>>>>> without restarting
>>>>>>
>>>>>> Jamil,
>>>>>>
>>>>>> Could you please take a look at this update, and confirm if this 
>>>>>> update doesn't break the original test scenarios?
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162484
>>>>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8162484/webrev.00/
>>>>>>
>>>>>> Artem
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20160812/28a09850/attachment.htm>

More information about the security-dev mailing list