Code Review Request: 8028562

[zaiyao liu]

Just record this email in security-dev alias.
On 2013/12/6 18:23, Xuelei Fan wrote:
> On 12/6/2013 6:04 PM, Xuelei Fan wrote:
>> On 12/6/2013 5:11 PM, zaiyao liu wrote:
>>> Hi Xuelei,
>>>
>>> Can you help me to post the webrev into  cr.openjdk.java.net if you
>>> think fine, I don't have account, I asked Eric to do this before, but he
>>> has left office.
>>>
>> Sure, but I'm not sure whether JDK 9 repository is open.  I will check
>> it and push the fix if the repository opened.
>>
> I think the JDK 9 repository has not open. We have to wait for a while.
>
> Xuelei
>
>> Thanks,
>> Xuelei
>>
>>> Thanks so much.
>>>
>>> Kevin
>>>
>>> On 2013/12/6 16:51, Xuelei Fan wrote:
>>>> Hi Kevin,
>>>>
>>>> Please won't mind there is a lot comments back-and-forth for such a
>>>> simple fix. That's the way we (dev) are working for better quality of
>>>> such a widely deployed platform.
>>>>
>>>> As this is the first bug you addressed in dev lib, I think it might be
>>>> help to have a good start rather than just fix the bug quickly.  After
>>>> you working over one or two JDK release, we may just say "looks fine to
>>>> me" for simple fixes. ;-)
>>>>
>>>> Hope you understand.
>>>>
>>>> Xuelei
>>>>
>>>> On 12/6/2013 4:41 PM, Xuelei Fan wrote:
>>>>> I just noticed this. "read error" does not describe the issue properly.
>>>>>    The underlying issue is that client message may be fragmented into
>>>>> small pieces during the TCP transaction. It's reasonable although it is
>>>>> pretty hard to reproduce.
>>>>>
>>>>> How about:
>>>>> - // will try to read one more time if there is a read error
>>>>> + // will try to read one more time in case client message
>>>>> + // is fragmented to multiple pieces
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 12/6/2013 3:45 PM, zaiyao liu wrote:
>>>>>> Hi Sean,
>>>>>>
>>>>>> Thanks for your suggestion, updated please review:
>>>>>> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>
>>>>>> Thanks again,
>>>>>>
>>>>>> Kevin
>>>>>> On 2013/12/6 1:13, Sean Mullan wrote:
>>>>>>> Sorry for the late comment, but there is a typo in this comment (roud
>>>>>>> -> round):
>>>>>>>
>>>>>>> // will try to read one more roud when read error
>>>>>>>
>>>>>>> I suggest rewording this to:
>>>>>>>
>>>>>>> // will try to read one more time if there is a read error
>>>>>>>
>>>>>>> Also, it is too late to push this for JDK 8 as it is not critical, so
>>>>>>> you will have to wait until the JDK 9 repo opens ...
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sean
>>>>>>>
>>>>>>> On 12/04/2013 07:45 PM, zaiyao liu wrote:
>>>>>>>> Hi Xuelei,
>>>>>>>>
>>>>>>>> Can you help to submit this change to repository? I don't have
>>>>>>>> openjdk
>>>>>>>> account.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> Kevin
>>>>>>>> On 2013/12/4 15:41, Xuelei Fan wrote:
>>>>>>>>> Looks fine to me.
>>>>>>>>>
>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>> On 12/4/2013 3:24 PM, zaiyao liu wrote:
>>>>>>>>>> Hi Xuelei,
>>>>>>>>>>
>>>>>>>>>> I have updated, please
>>>>>>>>>> review:http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>>>>>
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eewang/kevin/JDK-8028562/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>>> Kevin
>>>>>>>>>> On 2013/12/4 14:50, Xuelei Fan wrote:
>>>>>>>>>>> On 12/4/2013 2:36 PM, zaiyao liu wrote:
>>>>>>>>>>>> Hi Xuelei,
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for you suggestion. please review again:
>>>>>>>>>>>> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>>>>>>>
>>>>>>>>>>> Need a white space:
>>>>>>>>>>> - 224   //will try to read one more roud when read error
>>>>>>>>>>> + 224   // will try to read one more roud when read error
>>>>>>>>>>>
>>>>>>>>>>> The message is not clear enough:
>>>>>>>>>>> - 302  log("will read one more round");
>>>>>>>>>>> + 302  log("Need to read more from client");
>>>>>>>>>>>
>>>>>>>>>>> Otherwise, looks fine to me.  Please go ahead.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Xuelei
>>>>>>>>>>>
>>>>>>>>>>>> Kevin
>>>>>>>>>>>> On 2013/12/4 12:06, Xuelei Fan wrote:
>>>>>>>>>>>>> On 12/4/2013 11:33 AM, zaiyao liu wrote:
>>>>>>>>>>>>>> Hi Xuelei,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can you help to review again.
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ewang/kevin/JDK-8028562/webrev.00/
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for the update.  Please pay attentions to the code
>>>>>>>>>>>>> conversions.
>>>>>>>>>>>>>
>>>>>>>>>>>>>       300 if (serverIn.remaining() != clientMsg.length) {
>>>>>>>>>>>>>       301     if(retry){
>>>>>>>>>>>>>       302        log("will read one more round");
>>>>>>>>>>>>>
>>>>>>>>>>>>> It might be reasonable to retry when "serverIn.remaining()" less
>>>>>>>>>>>>> than
>>>>>>>>>>>>> clientMsg.length", what do you think?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Xuelei
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Kevin
>>>>>>>>>>>>>> On 2013/12/3 19:50, Xuelei Fan wrote:
>>>>>>>>>>>>>>> On 12/3/2013 6:59 PM, zaiyao liu wrote:
>>>>>>>>>>>>>>>> Hi Xuelei,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I can't reproduce this issue after run 900 times at
>>>>>>>>>>>>>>>> windows and
>>>>>>>>>>>>>>>> linux
>>>>>>>>>>>>>>>> platform,
>>>>>>>>>>>>>>> It should be pretty hard to reproduce the issue in normal
>>>>>>>>>>>>>>> TCP/IP
>>>>>>>>>>>>>>> environment.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> for this fix just run one more round after get exception.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> please review:
>>>>>>>>>>>>>>>> http://sqeweb.us.oracle.com/net/sqenfs-1/export1/comp/jsn/users/kevin1/webrev/8028562/webrev/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I don't think it is the expected fix.  Looks like the
>>>>>>>>>>>>>>> underlying
>>>>>>>>>>>>>>> issue
>>>>>>>>>>>>>>> is that "serverOut.remaining() == 0" (line 282) does not
>>>>>>>>>>>>>>> always
>>>>>>>>>>>>>>> mean
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> server has received all of the client message (line 298,
>>>>>>>>>>>>>>> (serverIn.remaining() != clientMsg.length)).  I would suggest
>>>>>>>>>>>>>>> run one
>>>>>>>>>>>>>>> more round (at line 241) after server message delivered
>>>>>>>>>>>>>>> ("serverOut.remaining() == 0" (line 282)).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The logic looks like, in runTest(boolean):
>>>>>>>>>>>>>>> loop (line 241):
>>>>>>>>>>>>>>>          read client message
>>>>>>>>>>>>>>>          send server message
>>>>>>>>>>>>>>>          if server delivered all server message {
>>>>>>>>>>>>>>>              if server received all client message {
>>>>>>>>>>>>>>>                  check the message
>>>>>>>>>>>>>>>              } else {
>>>>>>>>>>>>>>>                  loop one more time, go to "loop" (only one
>>>>>>>>>>>>>>> time?).
>>>>>>>>>>>>>>>              }
>>>>>>>>>>>>>>>          }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hope it helps.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Xuelei
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Kevin