[RFR] 8184328: JDK8u131 socketRead0 hang at SSL read
Xuelei Fan
xuelei.fan at oracle.com
Fri Sep 15 14:32:21 UTC 2017
On 9/15/2017 7:16 AM, Rob McKenna wrote:
> On 13/09/17 03:52, Xuelei Fan wrote:
>>
>>
>> On 9/13/2017 8:52 AM, Rob McKenna wrote:
>>> Hi Xuelei,
>>>
>>> This behaviour is already exposed via the autoclose boolean in:
>>>
>>> https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean-
>>>
>> I did not get the point. What do you mean by this behavior is already
>> exposed?
>
> In SSLSocketImpl.closeSocket() waitForClose is only called if autoclose
> is true. If not the SSLSocket simply calls super.close().
>
Did you get something different? I think waitForClose is only called if
autoclose is false.
No matter the autoclose is true or false, I'm not sure what do you mean
by this behavior is already exposed. Can you describe more about the point.
>>
>>> My position would be that allowing 5 retries allows us to say with some
>>> confidence that we're not going to get a close_notify from the server.
>> You have more chance to get the close_notify, but it does not mean you can
>> always get the close_notify in 5 retries. When you cannot get it, something
>> bad happens.
>
> No, the property would need to be tuned to suit the networking
> environment in which the application is deployed. Much the same as a
> timeout would be.
>
>>
>>> If this is the case I think its reasonable to close the connection.
>>>
>>> W.r.t. a separate timeout, the underlying mechanics of a close already
>>> depend on the readTimeout in this situation. (waiting on a close_notify
>>> requires performing a read so the read timeout makes sense in this
>>> context) I'm happy to alter that but I think that the combination of
>>> a timeout and a retry count is straightforward and lower impact.
>>>
>>> In my opinion the default behaviour of potentially hanging indefinitely
>>> is worse than the alternative here. (bearing in mind that we are closing
>>> the underlying socket)
>>>
>> I did not get the point, are we really closing the underlying socket (or the
>> layered ssl connection?) for the context of you update?
>
> We're calling fatal which calls closeSocket which in turn calls
> super.close(). (this calls Socket.close() via BaseSSLSocketImpl /
> SSLSocket) As noted in an earlier reply, this will close the
> underlying native socket. (I'll perform more testing to verify this)
>
When the fatal get called? I may miss something. Could you describe
the scenarios in more details?
Xuelei
> -Rob
>
>>
>> Xuelei
>>
>>> I'll file a CSR as soon as we settle on the direction this fix will
>>> take.
>>>
>>> -Rob
>>>
>>> On 13/09/17 05:52, Xuelei Fan wrote:
>>>> In theory, there are intermittent compatibility problems as this update may
>>>> not close the SSL connection over the existing socket layer gracefully, even
>>>> for high speed networking environments, while the underlying socket is
>>>> alive. The impact could be serious in some environment.
>>>>
>>>> For safe, I may suggest turn this countermeasure off by default. And
>>>> providing options to turn on this countermeasure:
>>>> 1. Close the SSL connection gracefully by default; or
>>>> 2. Close the SSL connection after a timeout.
>>>>
>>>> It's hardly to say 5 times receiving timeout is better/safer than timeout
>>>> once in this context. As you have already had a system property to control,
>>>> you may be able to use options other than the customized socket receiving
>>>> timeout, so that the closing timeout is not mixed/confused/dependent on/with
>>>> the receiving timeout.
>>>>
>>>> Put all together:
>>>> 1. define a closing timeout, for example "jdk.tls.waitForClose".
>>>> 2. the property default value is zero, no behavior changes.
>>>> 3. applications can set positive milliseconds value for the property. The
>>>> SSL connection will be closed in the set milliseconds (or about the maximum
>>>> value between SO_TIMEOUT and closing timeout), the connection is not grant
>>>> to be gracefully.
>>>>
>>>> What do you think?
>>>>
>>>> BTW, please file a CSR as this update is introducing an external system
>>>> property.
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 9/11/2017 3:29 PM, Rob McKenna wrote:
>>>>> Hi folks,
>>>>>
>>>>> In high latency environments a client SSLSocket with autoClose set to false
>>>>> can hang indefinitely if it does not correctly recieve a close_notify
>>>> >from the server.
>>>>>
>>>>> In order to rectify this situation I would like to suggest that we
>>>>> implement an integer JDK property (jdk.tls.closeRetries) which instructs
>>>>> waitForClose to attempt the close no more times than the value of the
>>>>> property. I would also suggest that 5 is a reasonable default.
>>>>>
>>>>> Note: each attempt times out based on the value of
>>>>> Socket.setSoTimeout(int timeout).
>>>>>
>>>>> Also, the behaviour here is similar to that of waitForClose() when
>>>>> autoClose is set to true, less the retries.
>>>>>
>>>>> http://cr.openjdk.java.net/~robm/8184328/webrev.01/
>>>>>
>>>>> -Rob
>>>>>
More information about the security-dev
mailing list