RFR 8071660 :URLPermission not handling empty method lists correctly

Pavel Rappo pavel.rappo at oracle.com
Mon Jun 20 13:57:02 UTC 2016


Vyom, please consider the following changes:

1. Append 8071660 to the lists of tests here:

	* @bug 8010464 8027570 8027687 8029354 8071660
                                               ^
2. Reformat the code the way it's in the rest of the file:

from:

 266         URLPermission that = (URLPermission)p;
 267         
 268         if(this.methods.isEmpty() && !that.methods.isEmpty())
 269             return false;
 270         
 271         if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") &&
 272                 Collections.indexOfSubList(this.methods, that.methods) == -1) {
 273             return false;
 274         }

to:

 266         URLPermission that = (URLPermission)p;
 267         
 268         if (this.methods.isEmpty() && !that.methods.isEmpty()) {
 269             return false;
 270         }
 271         
 272         if (!this.methods.isEmpty() &&
 273             !this.methods.get(0).equals("*") &&
 274             Collections.indexOfSubList(this.methods,
 275                                        that.methods) == -1) {
 276             return false;
 277         }

Other than these minor things, looks good.

Thanks,
-Pavel

> On 20 Jun 2016, at 12:36, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 
> On 17/06/16 15:09, Vyom Tewari wrote:
>> Hi,
>> 
>> Please find the latest
>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html
>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I
>> fixed the typo along with spaces issue.
> 
> Looks good!
> 
> -- daniel
> 
>> 
>> Thanks,
>> Vyom
>> 
>> 
>> On Friday 17 June 2016 06:34 PM, Roger Riggs wrote:
>>> Hi Vyom,
>>> 
>>> URLPermissionTest.java:
>>> 
>>> line 125:  it looks odd to assign the same variable twice with the
>>> same value.
>>>   Perhaps it should have been assigning this.url2 = url2.
>>> 
>>> line 330-332: please add a space after "," in argument lists.
>>> 
>>> Roger
>>> 
>>> 
>>> 
>>> 
>>> On 6/17/2016 8:42 AM, Daniel Fuchs wrote:
>>>> On 17/06/16 13:25, Vyom Tewari wrote:
>>>>> Hi Daniel,
>>>>> 
>>>>> thanks for review please find the latest
>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html
>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) .
>>>>> I added some more tests where base url is different.
>>>> 
>>>> Thanks Vyom,
>>>> Looks good to me.
>>>> 
>>>> See if you can get someone more experienced in the area
>>>> to give a thumb up too :-)
>>>> 
>>>> best regards,
>>>> 
>>>> -- daniel
>>>> 
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Vyom
>>>>> 
>>>>> On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote:
>>>>>> On 17/06/16 09:21, Vyom Tewari wrote:
>>>>>>> Hi All,
>>>>>>> 
>>>>>>> Please find the new
>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html
>>>>>>> 
>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>).
>>>>>>> 
>>>>>>> I  addressed the review comments given by Daniel.
>>>>>> 
>>>>>> Hi Vyom,
>>>>>> 
>>>>>> Looks good to me - but I'm a bit concerned that the previous
>>>>>> mistake was not caught that by any test.
>>>>>> Could you add a test that fails with you previous fix
>>>>>> but passes with the new one?
>>>>>> 
>>>>>> best regards,
>>>>>> 
>>>>>> -- daniel
>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> Vyom
>>>>>>> 
>>>>>>> 
>>>>>>> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote:
>>>>>>>> Hi Vyom,
>>>>>>>> 
>>>>>>>> This looks strange to me:
>>>>>>>> 
>>>>>>>> 268         if(!this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>> 269             return true;
>>>>>>>> 270         if(this.methods.isEmpty() && !that.methods.isEmpty())
>>>>>>>> 271             return false;
>>>>>>>> 272         if(this.methods.isEmpty() && that.methods.isEmpty())
>>>>>>>> 273             return true;
>>>>>>>> 
>>>>>>>> Namely, lines 269 & 273 will return true before the URL part
>>>>>>>> of the permission has been checked.
>>>>>>>> Is that really the expected behavior?
>>>>>>>> 
>>>>>>>> best regards,
>>>>>>>> 
>>>>>>>> -- daniel
>>>>>>>> 
>>>>>>>> On 11/06/16 05:50, vyom wrote:
>>>>>>>>> Hi All,
>>>>>>>>> 
>>>>>>>>> Please review the below fix.
>>>>>>>>> 
>>>>>>>>> Bug       :         JDK-8071660 URLPermission not handling empty
>>>>>>>>> method
>>>>>>>>> lists correctly
>>>>>>>>> Webrev :
>>>>>>>>> http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html
>>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html>
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Vyom
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 



More information about the net-dev mailing list