RFR(S): 8202289: Non-empty directory in module path is not handled properly at CDS/AppCDS dump time

Ioi Lam ioi.lam at oracle.com
Tue May 8 21:24:51 UTC 2018 

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.

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