RFR [13] JDK-8219389: Delegated task created by SSLEngine throws BufferUnderflowException

[Xuelei Fan]

> On Feb 19, 2019, at 10:06 PM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
> 
>> On 2/19/19 9:38 PM, Xuelei Fan wrote:
>> Hi Tony or Jamil,
>> Would you please review the following update:
>>    http://cr.openjdk.java.net/~xuelei/8219389/webrev.00/
>> BufferUnderflowException might be thrown if the record format does not confirm to the formal protocol syntax. The original bug was reported for the ClientHello handshake message, and was fixed in JDK-8215790.  I made an enhancement so that more handshake messages buffer operating RuntimeException could be handled properly.
>> Thanks,
>> Xuelei
> 
> I'm not saying your approach is incorrect, but I have to wonder if this is too generic.  Are you trying to catch situations other than RandomCookie throwing an exception?  
Yes, it is for cases other than RandomCookie.  Every call to ByteBuffer.get() could throw BUFE unless the length get checked.  The impl code did not always check the buffer length before calling the get() method.

The issues happens only when the record format does not comply to the protocol syntax.  For example, the record length is 5, but the content only have 1 byte.

We can fix these issues with two approaches: checking the buffer length before call get() and put(); or catch the exception in a general way.

> RandomCookie is only in ClientHello and ServerHello, so PostHandshakeContext doesn't look necessary.
> 
> If we need a generic catch all consumed extensions, why not just have it catch "Exception" and run fatal.  
I did not get the point.  It is not the purpose to catch all consumed exceptions, some of which could have been handled.

> This could eliminate many of the current fatal calls in the code and centralize the SSLException messages in these two files.  Assuming I understand the idea around this change correctly.
> 
We can go with checking the buffer length before each call to get(), which means we need to update a lot of code, and make no missing and mistakes.  Using a centralized way is more robust as if we missed somewhere in the handshake message operations like the ClientHello case.

I’m not sure I get the point about the elimination of fatal calls.  I think it is still needed to check the buffer length as soon as possible instead of depending on the two lazy centralized catch, which is used just in case the checking is missed somehow.

Thanks,
Xuelei

> Tony