I understand. The change looks fine.
Tony
On 2/19/19 10:32 PM, Xuelei Fan wrote:
On Feb 19, 2019, at 10:06 PM, Anthony Scarpino <[email protected]>
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
