Code Review Request: 8028562
Xuelei Fan
xuelei.fan at oracle.com
Mon Dec 16 03:33:42 UTC 2013
You are using the master repository. I think Dev is expected to used
the dev repository, http://hg.openjdk.java.net/jdk9/dev.
No additional code review required, I think. I will integrated the
changeset.
Thanks,
Xuelei
On 12/16/2013 10:45 AM, zaiyao liu wrote:
> 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