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 00:37:13 UTC 2018
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