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

Fri Aug 12 22:25:03 UTC 2016

Thank you for review Jamil.

Xuelei,

Could you please take a look?

Artem

On 08/12/2016 02:38 PM, Jamil Nimeh wrote:
> 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/7af17914/attachment.htm>