RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check
Ioi Lam
ioi.lam at oracle.com
Tue May 15 23:37:14 UTC 2018
On 5/15/18 4:33 PM, Ioi Lam wrote:
> Hi Jiangli,
>
> 245 // We need to validate the runtime modules image file size
> against the archived
> 246 // size information, obtain the runtime modules image path. The
> recorded dump
> 247 // time modules image path in the archive may be different from
> the runtime path
> 248 // if the JDK image has beed moved after generating the archive.
> 249 if (ClassLoader::is_modules_image(name)) {
> 250 name = ClassLoader::get_jrt_entry()->name();
> 251 }
>
> What happens if someone has a JAR file called foo.modules, and dumped
> with
>
> java -Xshare:dump -cp foo.modules
>
>
Another test case would be:
java -Xshare:dump -cp hello.jar:$JAVA_HOME/lib/modules:hi.jar
Again, I am not sure what the behavior should be, but calling
ClassLoader::is_modules_image(name) here makes it even confusing what
you're actually checking.
Thanks
- Ioi
> I think it's better to check that it's #0 in the classpath so we know
> for sure it's the system modules file.
>
> There's a similar problem here:
>
> 220 if (!ClassLoader::is_modules_image(name)) {
>
> In the following, instead of doing a copy, maybe it's better to add a
> length argument for os::file_name_strcmp, since
> sharedPathsMiscInfo.cpp is the only place in the VM that calls this
> function.
>
> 162 if (relaxed_check) {
> 163 // only check the begining portion of the runtime boot
> path, up to
> 164 // the length of the dump time boot path
> 165 size_t len = strlen(path);
> 166 runtime_boot_path = NEW_RESOURCE_ARRAY(char, len + 1);
> 167 strncpy(runtime_boot_path, Arguments::get_sysclasspath(),
> len);
> 168 runtime_boot_path[len] = '\0';
> 169 } else {
> 170 // check the full runtime boot path
> 171 runtime_boot_path = Arguments::get_sysclasspath();
> 172 }
> 173
> 174 // Do a quick check first with a simple
> 175 // strcmp(dump_time_boot_path, runtime_boot_path). If the
> paths are the
> 176 // same, the check has succeeded.
> 177 if (os::file_name_strcmp(path, runtime_boot_path) == 0) {
> 178 break; // ok
> 179 }
>
>
> Also, the copy could be wrong for the following output:
>
> path = /tmp/foo/modules
> Arguments::get_sysclasspath() = /tmp/foo/modulesxyz:/aaa:/bbb
>
> What is runtime_boot_path supposed to contain here? I am not sure if
> this code gives the intended output or not, but it's not easy to
> understand what it actually does by operating on an truncated pathname.
>
> String manipulation code is always hard to read. I would suggest
> braking it up into smaller functions:
>
> 1. Get the first entry of dump time and run time boot path -> d0 and r0
> -> ignore d0
> -> r0 same as ClassLoader::get_jrt_entry()->name();
>
> 2. Get the remaining part of the dump time and run time boot path ->
> d_remain, r_remain
> -> relaxed_check: d_remain must be a prefix of r_remain
> -> !relaxed_check: They must be identical.
>
>
> By the way, I think the test case in JDK-8202935 should be reviewed
> together inside this RFR, since the test validates the feature
> implemented here. I don't think we need a separate bug ID. Otherwise
> it will be hard to track the test cases.
>
>
> Thanks
> - Ioi
>
>
> On 5/13/18 5:31 PM, Jiangli Zhou wrote:
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~jiangli/8199807/webrev.01/
>>
>> 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