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

[Xuelei Fan]

On 7/3/2012 10:40 AM, Weijun Wang wrote:
> No new test needed. I only think that you might be able to hack the
> current test a little to reproduce this and see if the failure is the
> same and if your code change can fix it. There is no need to keep this
> hack in your final changeset.
> 
I tied several different approaches within this test, but failed to
reproduce the abbreviated handshaking. ;-) It is not easy to hack the
test without significant changes.

Xuelei

> -Max
> 
> 
> On 07/03/2012 10:37 AM, Xuelei Fan wrote:
>> 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
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>