Code Review Request: 8028562
zaiyao liu
zaiyao.liu at oracle.com
Mon Dec 16 02:45:46 UTC 2013
Hi Andrew and Sean,
Please help to review,this webrev based on JDK9:
http://cr.openjdk.java.net/~pzhang/Kevin/8028562/webrev/
Thanks and Regards.
Kevin
On 2013/12/16 9:40, zaiyao liu wrote:
> 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
>
More information about the security-dev
mailing list