Code review request, CR 7180038 regression test failure, SSLEngineBadBufferArrayAccess.java

[Xuelei Fan]

On 7/3/2012 10:02 AM, Weijun Wang wrote:
> 
> 
> On 07/03/2012 09:48 AM, Xuelei Fan wrote:
>> On 7/2/2012 4:35 PM, Weijun Wang wrote:
>>> I take a look at the test output. When the last handshake starts:
>>>
>>> ================
>>> server unwrap: OK/NEED_TASK, 230/0 bytes
>>>      running delegated task...
>>>      new HandshakeStatus: NEED_WRAP
>>> ----
>>> server wrap: OK/NEED_WRAP, 0/86 bytes
>>> ================
>>>
>>> Here the first wrap only generates 86 bytes, I guess that's the
>>> ServerHello message? It keeps the state at NEED_WRAP but then never
>>> really generates the Certificate message. What might be the problem?
>>>
>> Good catch!
>>
>> It was the abbreviated handshaking. I guess that the previous client has
>> not closed the socket completely, so for *this* handshaking, the
>> abbreviated handshaking rather than the full handshaking is used.
>>
>> For full handshaking, it is the client sending the "Finished" message at
>> first. However, for abbreviated handshaking, the server send the
>> "Finished" message at first.
>>
>> In the current scenarios, it is expected that the client sends its
>> application data (26 bytes), and then the server sends its application
>> data (29 bytes). However, the abbreviated handshaking disorder the
>> sequence in that it is the sever sends it application data (29 bytes)
>> before client. In such cases, the following logic does not stand any
>> more:
>>      if (!closed && (serverOut.remaining() == 0)) {
>>         closed = true;
>>         ...
>>         if (serverIn.remaining() != clientMsg.length) {
>>
>>             throw new Exception("Client:  Data length error");
>>         }
>>         ...
>>      }
>>
>> Because the server has not receive the client message when the server
>> sends its application data.
>>
>> I think it is a test issue, the current fix should has already addressed
>> the issue.
> 
> That's great.
> 
> Since the reason is clear, can you reproduce this failure and then
> confirm the current fix does solve the problem?
> 
It is possible to reproduce this failure with a new test case. But it is
pretty hard to reproduce it within this test. I was wondering as it is
test bug, so we may not want a extra test case to prove that this test
is correct.

We also have a nested remind that, "IF THIS FAILS, PLEASE REPORT THIS TO
THE SECURITY TEAM.  WE HAVE BEEN UNABLE TO RELIABLY DUPLICATE." I think
it might be OK that we do not reproduce this issue at present.

What do you think?

Thanks,
Xuelei

> Thanks
> Max
> 
>>
>> Xuelei
>>
>>> Thanks
>>> Max
>>>
>>> On 07/02/2012 10:39 AM, Xuelei Fan wrote:
>>>> Hi Weijun,
>>>>
>>>> Would you please review the test update for CR 7180038?
>>>>       http://cr.openjdk.java.net./~xuelei/7180038/webrev.00/
>>>>
>>>> We cannot reproduce the issue. However, from the test log, there is two
>>>> possible issues exposed by this CR.
>>>> 1. the improper test case senarios of un/wrap()
>>>>      In the test case, the scenarios is
>>>> unwrap()->wrap()->serverOut.remaining()->"serverIn.remaining() !=
>>>> clientMsg.length". After the wrap(), the next operation may need to be
>>>> unwrap() to get more incoming data before comparing serverIn buffer
>>>> with
>>>> the expected client message.
>>>>
>>>>       This fix is trying to do the comparing after the engine has
>>>> closed.
>>>>
>>>> 2. From the log, the engine status and handshaking status move from
>>>> CLOSED/NOT_HANDSHAKING to OK/FINISHED. FINISHED means the TLS
>>>> handshaking just finished. As the handshaking should have completed for
>>>> a while, it does not sound like a correct status change.
>>>>
>>>>       However, I did not find why this happens. Need more info. So I
>>>> added
>>>> a line of log (suggested by Brad Wetmore) to collect the next failure:
>>>>
>>>>       IF THIS FAILS, PLEASE REPORT THIS TO THE SECURITY TEAM.  WE HAVE
>>>>       BEEN UNABLE TO RELIABLY DUPLICATE.
>>>>
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>
>>
>>
>