[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 19:02:18 UTC 2016 

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

More information about the security-dev mailing list