RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check
Calvin Cheung
calvin.cheung at oracle.com
Tue May 15 00:31:45 UTC 2018
On 5/13/18, 5:31 PM, Jiangli Zhou wrote:
> Here is the updated webrev:
> http://cr.openjdk.java.net/~jiangli/8199807/webrev.01/
> <http://cr.openjdk.java.net/%7Ejiangli/8199807/webrev.01/>
Looks good.
Could you clarify the following comment in filemap.cpp?
245 // We need to validate the runtime modules image file size
against the archived
246 // size information, obtain the runtime modules image path.
I'd suggest something like:
// In order to validate the runtime modules image file size
against the archived
// size information, we need to obtain the runtime modules
image path.
I don't need to see another webrev for the above comment change.
thanks,
Calvin
>
> Thanks,
> Jiangli
>
>> On May 11, 2018, at 6:07 PM, Jiangli Zhou <jiangli.zhou at Oracle.COM>
>> wrote:
>>
>> Hi Calvin,
>>
>> Thanks for the review.
>>
>>> On May 11, 2018, at 4:32 PM, Calvin Cheung
>>> <calvin.cheung at oracle.com> wrote:
>>>
>>> Hi Jiangli,
>>>
>>> Thanks for doing this useful change.
>>>
>>> I have a few minor comments:
>>>
>>> 1) sharedPathsMiscInfo.cpp
>>>
>>> 199 dp ++;
>>>
>>> Should the above be similar to line 194 as follows?
>>> dp += path_sep_len;
>>
>> Good catch. Will fix.
>>
>>>
>>> 2) filemap.cpp
>>>
>>> 244 if (ClassLoader::is_modules_image(name)) {
>>>
>>> I think the above could be:
>>> if (is_modules_image()) {
>>>
>>
>> Let’s keep it this way as is_modules_image() is a wrapper of
>> 'ClassLoader::is_modules_image(name())’. We already have the ‘name’
>> in this case.
>>
>>> The is_modules_image() is a member function of SharedClassPathEntry.
>>>
>>> Btw, why is it necessary to get the runtime modules image path again
>>> in lines 244 - 246?
>>
>> As the runtime modules image path could be different from the dump
>> time if the JDK image is moved/copied after archive generation, we
>> need to make sure to use the correct file for file size check. The
>> recored path in the archive is the dump time modules image path,
>> which may no longer be existing at runtime. I will add some comments
>> if that’s helpful.
>>
>>>
>>> 3) BootClassPathMismatch.java
>>>
>>> Could you move line 103 to right above line 107 (opts.addPrefix(…))?
>>> Because the archive dumping part of the test doesn’t use appJar.
>>
>> Sure.
>>
>> Thanks!
>>
>> Jiangli
>>
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 5/11/18, 3:15 PM, Jiangli Zhou wrote:
>>>> I’ve withdrawn my original weberv and removed the build change.
>>>> Here is the updated webrev that only addresses 8199807.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~jiangli/8199807/webrev.00/
>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>> On May 11, 2018, at 2:44 PM, Jiangli Zhou<jiangli.zhou at Oracle.COM>
>>>>> wrote:
>>>>>
>>>>> Hi Erik,
>>>>>
>>>>> Thank you so much for investigating a proper fix for the
>>>>> vm_version.o issue. I will withdraw the build change from my
>>>>> original webrev.
>>>>>
>>>>> Thanks again for taking over the bug!
>>>>> Jiangli
>>>>>
>>>>>> On May 11, 2018, at 2:33 PM, Erik
>>>>>> Joelsson<erik.joelsson at oracle.com> wrote:
>>>>>>
>>>>>> Received: from [10.132.185.167] (/10.132.185.167)
>>>>>> by default (Oracle Beehive Gateway v4.0)
>>>>>> with ESMTP ; Fri, 11 May 2018 14:33:20 -0700
>>>>>> Subject: Re: RFR: 8199807& 8202738: AppCDS performs overly
>>>>>> restrictive path
>>>>>> matching check
>>>>>> To: Jiangli Zhou<jiangli.zhou at oracle.com>,
>>>>>> "hotspot-runtime-dev at openjdk.java.net runtime"
>>>>>> <hotspot-runtime-dev at openjdk.java.net>,
>>>>>> build-dev<build-dev at openjdk.java.net>
>>>>>> References:<B173B2EF-9908-418A-9DCC-EE6D9133DC10 at oracle.com>
>>>>>> From: Erik Joelsson<erik.joelsson at oracle.com>
>>>>>> Organization: Oracle Corporation
>>>>>> Message-ID:<ae4c66a1-ad7d-00f1-ced1-729704d56a9a at oracle.com>
>>>>>> Date: Fri, 11 May 2018 14:33:20 -0700
>>>>>> User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0)
>>>>>> Gecko/20100101 Thunderbird/52.7.0
>>>>>> MIME-Version: 1.0
>>>>>> In-Reply-To:<B173B2EF-9908-418A-9DCC-EE6D9133DC10 at oracle.com>
>>>>>> Content-Type: text/plain; charset=utf-8; format=flowed
>>>>>> Content-Transfer-Encoding: 8bit
>>>>>> Content-Language: en-US
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> For the build change, it's very undesirable to always have to
>>>>>> relink libjvm on every incremental build. Such a change cannot be
>>>>>> accepted.
>>>>>>
>>>>>> I have a counter suggestion, which is still a bit of a hack, but
>>>>>> it will cause vm_version.cpp to be recompiled (almost) every time
>>>>>> libjvm.so needs to be relinked. The drawback is that compiling
>>>>>> vm_version.cpp is now bound to happen absolutely last and so
>>>>>> cannot happen in parallel with other compilations.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~erikj/8202738/webrev.01/index.html
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>
>>>>>>> On 2018-05-10 16:11, Jiangli Zhou wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the following webrev that addresses the issue of
>>>>>>> copied/moved JDK image after generating a CDS archive. Thanks
>>>>>>> Karen Kinnear and Alan Baterman for initiating the
>>>>>>> investigation& discussions in this area (especially the ease of
>>>>>>> usage). Thanks Ioi for implementing a test case for moved JDK
>>>>>>> (JDK-8202935).
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~jiangli/8199807_8202738/webrev.00/
>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738?filter=14921
>>>>>>>
>>>>>>> The webrev includes the following three main parts:
>>>>>>>
>>>>>>> 1. Reduced check for JDK ‘modules’ image file at runtime. The
>>>>>>> runtime path to the ‘modules’ image is no longer required to the
>>>>>>> the same as dump time path. Runtime performs file size check
>>>>>>> only for the ‘modules’ image, which must match with the dump
>>>>>>> time ‘modules’ size. Invalidation of an outdated archive is
>>>>>>> achieved by the existing vm_version string check (the archived
>>>>>>> vm_version string must match with the runtime vm_version string).
>>>>>>>
>>>>>>> 2. Boot path check are now performed based on the content of the
>>>>>>> archive. Also added a new test case in
>>>>>>> BootClassPathMismatch.java and add more comments for existing
>>>>>>> test cases.
>>>>>>>
>>>>>>> 3. Fixed the stale vm_version string issue with incremental
>>>>>>> build. The issue was discovered during the work of 8199807. CDS
>>>>>>> uses vm_version string as part of the runtime validation check
>>>>>>> for archive. A stale vm_version string causes the CDS runtime to
>>>>>>> mistakenly accept an outdated archive. The fix is to make sure
>>>>>>> vm_version.o is recompiled properly when the library/vm is rebuilt.
>>>>>>>
>>>>>>> Tested with hs-tier1-4 and jdk-tier1-2. Tested by relocating the
>>>>>>> JDK image manually after generating an archive. Also tested with
>>>>>>> Ioi’s test both locally and via Mach5.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> For the build change, it's very undesirable to always have to
>>>>>> relink libjvm on every incremental build. Such a change cannot be
>>>>>> accepted.
>>>>>>
>>>>>> I have a counter suggestion, which is still a bit of a hack, but
>>>>>> it will cause vm_version.cpp to be recompiled (almost) every time
>>>>>> libjvm.so needs to be relinked. The drawback is that compiling
>>>>>> vm_version.cpp is now bound to happen absolutely last and so
>>>>>> cannot happen in parallel with other compilations.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~erikj/8202738/webrev.01/index.html
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>
>>>>>>> On 2018-05-10 16:11, Jiangli Zhou wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the following webrev that addresses the issue of
>>>>>>> copied/moved JDK image after generating a CDS archive. Thanks
>>>>>>> Karen Kinnear and Alan Baterman for initiating the
>>>>>>> investigation& discussions in this area (especially the ease of
>>>>>>> usage). Thanks Ioi for implementing a test case for moved JDK
>>>>>>> (JDK-8202935).
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~jiangli/8199807_8202738/webrev.00/
>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8199807?filter=14921
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8202738?filter=14921
>>>>>>>
>>>>>>> The webrev includes the following three main parts:
>>>>>>>
>>>>>>> 1. Reduced check for JDK ‘modules’ image file at runtime. The
>>>>>>> runtime path to the ‘modules’ image is no longer required to the
>>>>>>> the same as dump time path. Runtime performs file size check
>>>>>>> only for the ‘modules’ image, which must match with the dump
>>>>>>> time ‘modules’ size. Invalidation of an outdated archive is
>>>>>>> achieved by the existing vm_version string check (the archived
>>>>>>> vm_version string must match with the runtime vm_version string).
>>>>>>>
>>>>>>> 2. Boot path check are now performed based on the content of the
>>>>>>> archive. Also added a new test case in
>>>>>>> BootClassPathMismatch.java and add more comments for existing
>>>>>>> test cases.
>>>>>>>
>>>>>>> 3. Fixed the stale vm_version string issue with incremental
>>>>>>> build. The issue was discovered during the work of 8199807. CDS
>>>>>>> uses vm_version string as part of the runtime validation check
>>>>>>> for archive. A stale vm_version string causes the CDS runtime to
>>>>>>> mistakenly accept an outdated archive. The fix is to make sure
>>>>>>> vm_version.o is recompiled properly when the library/vm is rebuilt.
>>>>>>>
>>>>>>> Tested with hs-tier1-4 and jdk-tier1-2. Tested by relocating the
>>>>>>> JDK image manually after generating an archive. Also tested with
>>>>>>> Ioi’s test both locally and via Mach5.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>
More information about the hotspot-runtime-dev
mailing list