RFR(S): 8202289: Non-empty directory in module path is not handled properly at CDS/AppCDS dump time
Calvin Cheung
calvin.cheung at oracle.com
Thu May 10 18:00:01 UTC 2018
Thanks, Ioi.
Calvin
On 5/9/18, 8:51 PM, Ioi Lam wrote:
> Looks good. Thanks!
>
> - Ioi
>
>
> On 5/9/18 5:37 PM, Calvin Cheung wrote:
>>
>>
>> On 5/8/18, 2:24 PM, Ioi Lam wrote:
>>> I think we do need to check the results of get_canonical_path. On
>>> Linux, the canonical path can be longer than the source path, which
>>> means ENAMETOOLONG may be returned.
>>>
>>> http://hg.openjdk.java.net/jdk/hs/file/tip/src/java.base/unix/native/libjava/canonicalize_md.c#l253
>>>
>>>
>>> So instead of assert, it's best to check for failure and exit the
>>> dumping process.
>> I've made the above change.
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.04/
>>
>> thanks,
>> Calvin
>>>
>>> The rest of the changes look OK to me.
>>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>>
>>> On 5/8/18 10:41 AM, Calvin Cheung wrote:
>>>>
>>>>
>>>> On 5/7/18, 2:58 PM, Jiangli Zhou wrote:
>>>>>
>>>>>> On May 7, 2018, at 11:33 AM, Calvin Cheung
>>>>>> <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/7/18, 10:28 AM, Jiangli Zhou wrote:
>>>>>>> Hi Calvin,
>>>>>>>
>>>>>>> -classLoader.cpp
>>>>>>>
>>>>>>> The get_canonical_path() call for ‘path’ should be moved out of
>>>>>>> the ‘for' loop.
>>>>>>>
>>>>>>> 1569 get_canonical_path(path, canonical_src_path,
>>>>>>> JVM_MAXPATHLEN)) {
>>>>>> Fixed.
>>>>>>> To make the code more readable, please rename ‘canonical_path’
>>>>>>> to ‘canonical_path_table_entry’ (or
>>>>>>> ‘canonical_path_table_entry_name’)
>>>>>> I chose the shorter one.
>>>>>>> and ‘canonical_src_path’ to ‘canonical_class_src_path’.
>>>>>> Renamed.
>>>>>>> BTW, when we create the path entries, the paths are already
>>>>>>> converted to canonical forms. I think there is no need to call
>>>>>>> get_canonical_path() on ‘ent->name()’. Can you please double check?
>>>>>> The call to get_canonical_path() is only for the non-directory
>>>>>> case (please refer to ClassLoader::create_class_path_entry() for
>>>>>> details).
>>>>>> I did try get_canonical_path() for the directory case but it
>>>>>> didn't work during runtime; it failed in
>>>>>> SystemDictionaryShared::is_shared_class_visible_for_classloader()
>>>>>> in the following check:
>>>>>>
>>>>>> (strcmp(ent->name(),
>>>>>> ClassLoader::skip_uri_protocol(mod_entry->location()->as_C_string()))
>>>>>> == 0)) {
>>>>>>
>>>>>> The location() in the mod_entry isn't in canonical form. If we
>>>>>> convert it to canonical form during runtime, there maybe
>>>>>> performance impact.
>>>>>> I think the proper way of fixing it is storing the location() in
>>>>>> canonical form when a module entry is being defined. I'm not sure
>>>>>> if it is possible for the core lib to pass the location in
>>>>>> canonical form to jvm when a module entry is being defined.
>>>>>> I can file a follow-up RFE to address that.
>>>>>
>>>>> Please file a RFE.
>>>> I've filed JDK-8202750
>>>> <https://bugs.openjdk.java.net/browse/JDK-8202750>
>>>>>
>>>>> There is no need to check ‘res’ in very iteration since it doesn’t
>>>>> change after being initialized by get_canonical_path(). Also, I
>>>>> think we are not handling get_canonical_path() (both calls)
>>>>> failure properly. However, I question why get_canonical_path()
>>>>> would fail when given a valid path in this particular case. So I
>>>>> think we should either assert get_canonical_path() returns true,
>>>>> or report a fatal error for dumping.
>>>> I've made the change and added assert after get_canonical_path() at
>>>> both calls.
>>>>
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.03/
>>>>
>>>> thanks,
>>>> Calvin
>>>>>>> - os_windows.cpp
>>>>>>>
>>>>>>> Please fix indentation at line 4390& 4391.
>>>>>> That was done on purpose because the condition in line 4390 is
>>>>>> '||' with line 4391.
>>>>>> I'm further indenting line 4391 to make it clearer.
>>>>>>
>>>>>> 4389 if (search_path[1] == ':'&&
>>>>>> 4390 (search_path[2] == '\0' ||
>>>>>> 4391 (search_path[2] == '\\' <smb://%27> &&
>>>>>> search_path[3] == '\0'))) {
>>>>>
>>>>>
>>>>> Looks better.
>>>>>
>>>>>> Updated webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.02/
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>>>
>>>>>>>> On May 7, 2018, at 9:12 AM, Calvin
>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>
>>>>>>>> Hi Jiangli,
>>>>>>>>
>>>>>>>> On 5/3/18, 5:33 PM, Jiangli Zhou wrote:
>>>>>>>>> Hi Calvin,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On May 3, 2018, at 4:18 PM, Calvin
>>>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 5/3/18, 3:42 PM, Jiangli Zhou wrote:
>>>>>>>>>>> Hi Calvin,
>>>>>>>>>>>
>>>>>>>>>>> I think we don’t need to check the module path entries if
>>>>>>>>>>> only boot classes are archived. The ‘end’ is set to
>>>>>>>>>>> app_class_paths_start_index when there are only boot classes
>>>>>>>>>>> are archived. If there are app module classes loaded from
>>>>>>>>>>> jars/directories at dump time, has_platform_or_app_classes()
>>>>>>>>>>> should return true and ‘end’ is set to the end of the shared
>>>>>>>>>>> path table, which includes all module path entries. The
>>>>>>>>>>> following loop is not needed. The existing loop in the code
>>>>>>>>>>> covers all cases with different ‘end’ value based on the
>>>>>>>>>>> type of classes loaded in the archive.
>>>>>>>>>>> 366 // may need to check module path entries.
>>>>>>>>>>> 367 if ((end ==
>>>>>>>>>>> ClassLoaderExt::app_class_paths_start_index())&&
>>>>>>>>>>> (ClassLoader::num_module_path_entries()> 0)) {
>>>>>>>>>>> 368 for (int i =
>>>>>>>>>>> ClassLoaderExt::app_module_paths_start_index(); i<
>>>>>>>>>>> _shared_path_table_size; i++) {
>>>>>>>>>>> 369 SharedClassPathEntry* e = shared_path(i);
>>>>>>>>>>> 370 has_nonempty_dir = check_nonempty_dir(e);
>>>>>>>>>>>
>>>>>>>>>>> 371 }
>>>>>>>>>>> 372 }
>>>>>>>>>>>
>>>>>>>>>> The has_platform_or_app_classes() will return false if
>>>>>>>>>> ClassLoaderExt::record_result() hasn't been called while a
>>>>>>>>>> class is being loaded during dumping.
>>>>>>>>>> This could happen before this proposed change if one
>>>>>>>>>> specifies a --module-path with an non-empty directory
>>>>>>>>>> containing a module. While the class will be loaded during
>>>>>>>>>> dumping but its classpath_index will still be -1. Therefore,
>>>>>>>>>> the has_platform_or_app_classes() will return false and the
>>>>>>>>>> module path entries won't be checked without the above block
>>>>>>>>>> of code.
>>>>>>>>> With the module directory path being added to the path table
>>>>>>>>> now, it should record the correct path table index instead of
>>>>>>>>> -1 for module classes loaded from directory at dump time.
>>>>>>>>>
>>>>>>>>> I applied your patch and stepped through
>>>>>>>>> ClassLoader::record_result() with your test case. The
>>>>>>>>> following path comparison fails due the the extra ‘/‘ in
>>>>>>>>> ‘path’. That why it fails to find the correct entry in the
>>>>>>>>> path table, which causes the class type cannot be correctly
>>>>>>>>> identified.
>>>>>>>>>
>>>>>>>>> if (strcmp(canonical_path,
>>>>>>>>> os::native_path((char*)path)) == 0) {os::dir_is_empty
>>>>>>>>>
>>>>>>>>> (gdb) p canonical_path
>>>>>>>>> $74 = 0x7ffff0aea0f0
>>>>>>>>> “<snip>/jdk/open/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/JTwork/scratch/mods/com.simple"
>>>>>>>>> (gdb) p os::native_path((char*)path)
>>>>>>>>> $75 = 0x7ffff7fd4a65
>>>>>>>>> “<snip>/jdk/open/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/JTwork/scratch/mods/com.simple/"
>>>>>>>>>
>>>>>>>>> The trailing ‘/‘ should be excluded for the path comparison.
>>>>>>>>> JDK-8202519 is a related issue in the same area. It might be
>>>>>>>>> better to fix it together with this one.
>>>>>>>> Thanks for debugging the issue.
>>>>>>>> I've fixed it by calling get_canonical_path() on the "path"
>>>>>>>> before doing strcmp(). It should also fix the bug in
>>>>>>>> JDK-8202519 since a buffer is being allocated for
>>>>>>>> get_canonical_path() to write into. The os::native_path() on
>>>>>>>> linux does nothing and just returns the same string.
>>>>>>>>
>>>>>>>> I also discovered a problem in os::dir_is_empty() on windows.
>>>>>>>> When a directory path is passed into the function, it may have
>>>>>>>> the following form with forward slashes: C:/somedir
>>>>>>>> The fix is to call os::native_path() which essentially converts
>>>>>>>> the forward slashes to back slashes so that subsequent windows
>>>>>>>> api call returns the correct result.
>>>>>>>>
>>>>>>>>> Once the above issue is fixed, the class type should be
>>>>>>>>> recorded correctly for a module classes loaded from directory
>>>>>>>>> at dump time. Then ClassLoaderExt::record_result() can set
>>>>>>>>> ‘has_app_classes’ flag properly. And, no change should be
>>>>>>>>> needed in FileMapInfo::check_nonempty_dir_in_shared_path_table().
>>>>>>>> I don't need to touch filemap.cpp in the following updated webrev:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.01/
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> Calvin
>>>>>>>>>>> We also need a unit test for non-empty directory in module
>>>>>>>>>>> path. Please add.
>>>>>>>>>> It is already in my webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/test/hotspot/jtreg/runtime/appcds/jigsaw/modulepath/MainModuleOnly.java.sdiff.html
>>>>>>>>>>
>>>>>>>>> Ok. I was expecting a separate unit test. I’m fine with an
>>>>>>>>> added test case in MainModuleOnly.java.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Jiangli
>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>> Calvin
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jiangli
>>>>>>>>>>>
>>>>>>>>>>>> On May 3, 2018, at 3:24 PM, Calvin
>>>>>>>>>>>> Cheung<calvin.cheung at oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> bug:https://bugs.openjdk.java.net/browse/JDK-8202289
>>>>>>>>>>>> webrev:http://cr.openjdk.java.net/~ccheung/8202289/webrev.00/
>>>>>>>>>>>>
>>>>>>>>>>>> In ClassLoaderExt::process_module_table(), it adds all
>>>>>>>>>>>> entries from the ModuleEntryTable with the "file:" protocol
>>>>>>>>>>>> to the _module_path_entries and eventually to the
>>>>>>>>>>>> _shared_path_table.
>>>>>>>>>>>> The FileMapInfo::check_nonempty_dir_in_shared_path_table()
>>>>>>>>>>>> will check for non-empty directory in the module path.
>>>>>>>>>>>>
>>>>>>>>>>>> Ran all CDS and AppCDS tests locally on linux-x64.
>>>>>>>>>>>> Will run hs-tier{1,2,3} tests.
>>>>>>>>>>>>
>>>>>>>>>>>> thanks,
>>>>>>>>>>>> Calvin
>>>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list