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

[coleen.phillimore at oracle.com]

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());


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
>>>>>>>
>>>>>
>>