RFR: 8199807 & 8202738: AppCDS performs overly restrictive path matching check
Calvin Cheung
calvin.cheung at oracle.com
Mon May 21 17:52:03 UTC 2018
Hi Jiangli,
The latest changes look good.
Thanks for adding the test scenarios.
thanks,
Calvin
On 5/21/18, 10:12 AM, Jiangli Zhou wrote:
> After discussion with Ioi and Calvin, we all agreed the usages of os::file_name_strcmp() in SharedPathsMiscInfo::check() should be fixed as part of this change. Calvin (thanks!) also found an unlikely but possible edge case (-Xshare:dump -Xbootclasspath/a:foo.jar; -Xshare:on -Xbootclasspath/a:foo.jar123) that we need to handle. Added the test case in BootClassPathMismatch.java.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/%7Ejiangli/8199807/webrev.03/
>
> Thanks,
> Jiangli
>
>> On May 17, 2018, at 11:55 AM, Jiangli Zhou<jiangli.zhou at oracle.com> wrote:
>>
>> Thanks, Ioi! Will incorporate your suggestions below before integration.
>>
>> Jiangli
>>
>>> On May 17, 2018, at 11:52 AM, Ioi Lam<ioi.lam at oracle.com> wrote:
>>>
>>> Hi Jiangli,
>>>
>>> The changes look good. Just some minor nits. No need for new webrev if you decide to make these changes.
>>>
>>> // The first entry in boot path is the modules_image (guaranteed by
>>>
>>> // ClassLoader::setup_boot_search_path()). Skip the first entry. The
>>> // path of the runtime modules_image may be different from the dump
>>> // time path (e.g. the JDK image is copied to a different location
>>> // after generating the shared archive), which is acceptable. For most
>>> // common cases, the dump time boot path might contain modules_image only.
>>>
>>> Perhaps add an assert that #0 of the bootclasspath is the runtime module image?
>>>
>>>
>>> 444 isSigned = stream->check_is_signed();
>>> 445 if (isSigned) {
>>>
>>> Maybe remove isSigned variable and change to if (stream->check_is_signed()) ....
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 5/16/18 8:27 PM, Jiangli Zhou wrote:
>>>> Here is the updated webrev:
>>>> http://cr.openjdk.java.net/~jiangli/8199807/webrev.02/
>>>>
>>>> At CDS dump time, when SharedClassPathEntry::init() is called from FileMapInfo::allocate_shared_path_table(), we can identify the modules image entry directly. SharedClassPathEntry::init() now takes an extra bool argument to indicate if the current entry is the modules image. We no longer need to test the entry name using ClassLoader::is_modules_image(), which might be wrong for a shared path entry as Ioi pointed out.
>>>>
>>>> I added a ‘_type’ field in SharedClassPathEntry to record the type of shared path entries at dump time. At runtime, the path entry ‘_type’ is checked to identify the modules image in SharedClassPathEntry::validate(). SharedClassPathEntry::is_modules_image() no longer need to use ClassLoader::is_modules_image().
>>>>
>>>> Reworked the BOOT_PATH case in SharedPathsMiscInfo::check(). I left os::file_name_strcmp() unchanged since it’s also used by APP_PATH. I'll handle it in a separate REF to do more testing.
>>>>
>>>> I also included the MoveJDKTest.java written by Ioi in the updated webrev. In the test, I added a new case for a JAR entry named with Hello.modules.
>>>>
>>>> Reran all appcds tests including MoveJDKTest.java locally. All tests passed. Running tiered tests.
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>> On May 15, 2018, at 6:10 PM, Jiangli Zhou<jiangli.zhou at oracle.com> wrote:
>>>>>
>>>>> Hi Ioi,
>>>>>
>>>>> Thank you very much for the review. Good catches for those edge cases and thanks for the test cases. I will resolves them.
>>>>>
>>>>> Thanks!
>>>>> Jiangli
>>>>>
>>>>>> On May 15, 2018, at 4:33 PM, Ioi Lam<ioi.lam at oracle.com> 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
>>>>>>
>>>>>>
>>>>>> 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