Code Review Request, JDK-8214339 : SSLSocketImpl erroneously wraps SocketException

Xue-Lei Fan xuelei.fan at oracle.com
Fri Dec 14 21:46:22 UTC 2018


Hi,

The purpose of combination of the two lines together:

     shc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, "...");
     return null;    // make the complier happy

is actually for the reading of the code.  If one don't know the fatal() 
always throw an exception (compiler is one of them), he may continue 
reading the following code, and may be confusing about the code logic.

The "return" statement could never be executed actually.  It could be 
easier to catch the idea if using a format like:

    throw new SSLException("there is an alert")
-  return null;

But we need the fatal() method to wrap something more.

We used a lot comments similar to:

    return;     // fatal() always throws, make the compiler happy.

and the abbreviate comment:
    return;    // make the compiler happy

misses the part "fatal() always throws", and then it may look weird.

I'm fine to remove the comment, or use the full comment instead "fatal() 
always throws, make the compiler happy".  Which one is your preference?


On 12/14/2018 11:49 AM, Jamil Nimeh wrote:
> Looks pretty good.  I did have one question about a few of the methods 
> in KeyShareExtension and PreSharedKeyExtension, specifically where you 
> return nulls on error branches with the make-compiler-happy comments. In 
> those cases would it be a bit cleaner to set a byte[] variable to null 
> at the beginning of the method and then in the successful code path set 
> the value to whatever byte array comes back from hmacs or hkdf 
> operations?  At the end of the method you only need one return 
> statement, rather than a return null on every error branch, many of 
> which won't be executed due to method calls which always throw 
> exceptions.
Hm, I see your points.  As the return statement could never be executed, 
it may not worthy to declare the byte[] variable earlier.

See above. Is it a reasonable coding style to you as well?

Thanks,
Xuelei

> Not a big deal if you wish to leave things as they are 
> since I know the current approach is used in many places in the code 
> that this review doesn't touch.
> 
> --Jamil
> 
> On 12/14/2018 8:14 AM, Xue-Lei Fan wrote:
>> Hi,
>>
>> Please review the update:
>>    http://cr.openjdk.java.net/~xuelei/8214339/webrev.00/
>>
>> In some cases, the SSLProtocolException or SSLHandshakeException may 
>> be thrown if the underlying socket run into problems.  An application 
>> may depends on the exception class for further action, for example 
>> retry the connection with different parameters.
>>
>> This update is trying to separate the socket problem from the TLS 
>> protocol or handshake problem, by using different exception classes.
>>
>> Thanks,
>> Xuelei
> 


More information about the security-dev mailing list