[12] RFR: 8214827: Incorrect call ClassLoaders.toFileURL("jrt:/java.compiler")

[Jiangli Zhou]

On 1/9/19 6:35 PM, coleen.phillimore at oracle.com wrote:

>
> http://cr.openjdk.java.net/~jiangli/8214827/webrev.03/src/hotspot/share/classfile/systemDictionaryShared.cpp.udiff.html 
>
>
> + Handle protection_domain = Handle(THREAD, 
> mod->shared_protection_domain());
>
>
> Nit, you can write this as:
>
> + Handle protection_domain(THREAD, mod->shared_protection_domain());
>
Done.

Thanks!
Jiangli
>
> Not a full review and I don't need to see another webrev.
>
> Coleen
>
> On 1/9/19 9:25 PM, Calvin Cheung wrote:
>> Looks good.
>>
>> thanks,
>> Calvin
>>
>> On 1/9/19, 5:50 PM, Jiangli Zhou wrote:
>>> Hi Ioi and David,
>>>
>>> Thanks for the comments!
>>>
>>>
>>> On 1/9/19 5:13 PM, David Holmes wrote:
>>>> On 10/01/2019 10:47 am, Ioi Lam wrote:
>>>>>
>>>>> On 1/9/19 4:32 PM, Jiangli Zhou wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> Thanks for the review!
>>>>>>
>>>>>>
>>>>>> On 1/9/19 3:01 PM, Calvin Cheung wrote:
>>>>>>> Hi Jiangli,
>>>>>>>
>>>>>>> 1) systemDictionaryShared.cpp:
>>>>>>>
>>>>>>> Should all those CHECK_(protection_domain) be simply CHECK_NH?
>>>>>>> In get_shared_jar_manifest() of the same file, CHECK_NH is used. 
>>>>>>> But in other functions, CHECK_(something) is being used.
>>>>>>> Not sure which one is the preferred way?
>>>>>>
>>>>>> I'm not sure which is the preferred way either. I'm leaving it as 
>>>>>> is and also checking with Coleen...
>>>>>
>>>>> CHECK_(protection_domain)  depends on the fact that 
>>>>> protection_domain is NULL at the point of the CHECK_. This could 
>>>>> lead to maintenance issues down the road if someone decides to 
>>>>> initialize protection_domain earlier.
>>>>
>>>> I think this is fairly common practice though. We often define the 
>>>> "result" variable at the start of a method with an "error" value 
>>>> and return the result on error.
>>>
>>> I agree with David and it seems to be a common practice. However, I 
>>> don't have a strong preference on the usage of the CHECK_ macros. 
>>> Based on the feedback from Coleen, Calvin and Ioi, I updated the 
>>> webrev to use the CHECK_NH in 
>>> SystemDictionaryShared::get_shared_protection_domain(). The rest of 
>>> the usages in the file are not changed in this changeset to minimize 
>>> risks.
>>>
>>> Here is the updated webrev: 
>>> http://cr.openjdk.java.net/~jiangli/8214827/webrev.03/src/hotspot/share/classfile/systemDictionaryShared.cpp.frames.html
>>>
>>> Full webrev: http://cr.openjdk.java.net/~jiangli/8214827/webrev.03/
>>>
>>> Thanks!
>>> Jiangli
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> I think CHECK_NH is better. Easier to read and less dependencies.
>>>>>
>>>>> The rest of the changes and Calvin's suggestions look good to me.
>>>>>
>>>>> Thanks
>>>>>
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>> 2) ProtectionDomain.java:
>>>>>>>
>>>>>>> 58     TestCommon.run("-cp", appJar, 
>>>>>>> "ProtDomain").assertNormalExit();
>>>>>>>
>>>>>>>     You can also check the output by doing:
>>>>>>>     TestCommon.run("-cp", appJar, 
>>>>>>> "ProtDomain").assertNormalExit("Protection Domains match");
>>>>>>>     Similarly for line 61.
>>>>>>>
>>>>>>> 65     TestCommon.run("-cp", appJar, 
>>>>>>> "JimageClassProtDomain").assertNormalExit();
>>>>>>>
>>>>>>>      You can check the output "shouldNotContain" by doing:
>>>>>>>      TestCommon.run("-cp", appJar, "JimageClassProtDomain")
>>>>>>>          .assertNormalExit(output -> 
>>>>>>> output.shouldNotContain("Failed: Protection Domains do not match");
>>>>>>
>>>>>> I've updated the webrev and incorporated with your suggestions: 
>>>>>> http://cr.openjdk.java.net/~jiangli/8214827/webrev.02/.
>>>>>>>
>>>>>>> The remaining 3 files look good.
>>>>>>
>>>>>> Thanks!
>>>>>> Jiangli
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>
>>>>>>> On 1/9/19, 2:15 PM, Jiangli Zhou wrote:
>>>>>>>> Please review the bug fix for 
>>>>>>>> SystemDictionaryShared::get_shared_protection_domain(). When 
>>>>>>>> jrt:/<module> location is given, a null URL (due to the error 
>>>>>>>> in ClassLoaders.toFileURL) is used for the CodeSource, which 
>>>>>>>> results a wrong ProtectionDomain being used for the archived 
>>>>>>>> app/platform class from the runtime image. The granted 
>>>>>>>> permission is minimum in that case.
>>>>>>>>
>>>>>>>> This turns out to be an issue that's hidden for a long time. 
>>>>>>>> The JimageClassProtDomain.java test has been outdated and fails 
>>>>>>>> to detect the issue (I'm the guilt one). Please see the 
>>>>>>>> comments in the bug for more details. Thanks Martin for finding 
>>>>>>>> the issue, and David, Alan and Mandy for discussions.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8214827/webrev.01/
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8214827
>>>>>>>>
>>>>>>>> Tested with appcds tests locally. Tested tier1-2. Tier3 is 
>>>>>>>> still in progress.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>
>>>
>