[14] RFR(M): 8211723: AppCDS: referring to a jar file in any way other than exactly how it was done during dumping doesn't work

[Calvin Cheung]

On 6/26/19, 5:38 PM, Jiangli Zhou wrote:
> Hi Calvin,
>
> On Wed, Jun 26, 2019 at 3:49 PM Calvin Cheung<calvin.cheung at oracle.com>  wrote:
>> Hi Jiangli,
>>
>> Thanks for your review!
>>
>> On 6/26/19, 11:14 AM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> Thank you for working on this. Really glad to see this finally being
>>> done! I meant to review this earlier, but was delayed. Thank you for
>>> being patient.
>>>
>>> - src/hotspot/share/classfile/classLoader.hpp
>>>
>>> 960 bool ClassLoader::update_class_path_entry_list(const char *path,
>>>    961                                                bool check_for_duplicates,
>>>    962                                                bool is_boot_append,
>>>    963                                                bool from_class_path_attr,
>>>    964                                                bool throw_exception) {
>>> ...
>>>
>>>    984 #if INCLUDE_CDS
>>>    985     if (DumpSharedSpaces || DynamicDumpSharedSpaces) {
>>>    986       _shared_paths_misc_info->add_nonexist_path(path);
>>>    987     }
>>>    988 #endif
>>>
>>> The above code for _shared_paths_misc_info can be eliminated now. If a
>>> path does not exist at dump time but exists at runtime, it should be
>>> caught by the runtime shared path table check. I think the new check
>>> you added for the shared path table does cover that case. We should
>>> make sure we have sufficient test cases for that. It's good to also
>>> add comments to the shared path table check to describe all cases.
>> I don't think we can remove the above code because it will also update
>> the class path entry list for the jar files which are found from the
>> "Class-Path" attribute of a jar manifest.
>>
>> Below is the various call paths to the function (note the call from
>> process_jar_manifest):
>>
>> ClassLoader::update_class_path_entry_list(const char *, bool, bool,
>> bool, bool) : bool
>>       ClassLoader::setup_app_search_path(const char *) : void
>>       ClassLoader::setup_boot_search_path(const char *) : void
>>       ClassLoaderExt::process_jar_manifest(ClassPathEntry *, bool) : void
>>           ClassLoader::add_to_app_classpath_entries(const char *,
>> ClassPathEntry *, bool) : void
>>               ClassLoader::update_class_path_entry_list(const char *,
>> bool, bool, bool, bool) : bool
> I was referring to the #if INCLUDED_CDS code for
> _shared_paths_misc_info. It's no longer needed.
>
>     984 #if INCLUDE_CDS
>     985     if (DumpSharedSpaces || DynamicDumpSharedSpaces) {
>     986       _shared_paths_misc_info->add_nonexist_path(path);
>     987     }
>     988 #endif
>
> The rest of ClassLoader::update_class_path_entry_list() is still needed.
Without the above code, it will fail with the following sceanrio:
     non-existing jar during dump time but jar exists during runtime

I've added 2 test scenarios in the AppendClasspath.java test.

Updated webrevs:
     incremental: http://cr.openjdk.java.net/~ccheung/8211723/delta_01_02/
     full: http://cr.openjdk.java.net/~ccheung/8211723/webrev.02/
>
>>>
>>> - src/hotspot/share/memory/filemap.hpp
>>>
>>> 63   void init(const char* name, bool is_modules_image,
>>> ClassPathEntry* cpe, TRAPS);
>>>
>>> The 'name' argument can be removed since it can be retrieved from
>>> 'cpe', as you are passing the ClassPathEntry now.
>> I've made the change.
>>> - src/hotspot/share/classfile/sharedPathsMiscInfo.cpp
>>>
>>> With the removal of the problematic file name string comparison, the
>>> SharedPathsMiscInfo::check() becomes basically a no-op now. It should
>>> be removed now.
>>>
>>>    156 bool SharedPathsMiscInfo::check(jint type, const char* path, bool
>>> is_static) {
>>>    157   assert(UseSharedSpaces, "runtime only");
>>>    158   switch (type) {
>>>    159   case BOOT_PATH:
>>>    160     break;
>>>    161   case NON_EXIST:
>>>    162     {
>>>    163       struct stat st;
>>>    164       if (os::stat(path,&st) == 0) {
>>>    165         // The file actually exists
>>>    166         // But we want it to not exist ->   fail
>>>    167         return fail("File must not exist");
>>>    168       }
>>>    169     }
>>>    170     break;
>>>    171   case APP_PATH:
>>>    172     break;
>>>    173   default:
>>>    174     return fail("Corrupted archive file header");
>>>    175   }
>>>    176
>>>    177   return true;
>>>    178 }
>>>
>>> The following function should also be removed:
>>>    107 bool SharedPathsMiscInfo::check(bool is_static) {
>> The above functions can't be removed yet due to the same reasoning as
>> above. It ensures the non-existing of the jar file(s) found in the
>> "Class-Path" attributes.
> I think we can enhance your current change to record the
> 'non-existing' manifest Class-Path header referenced JAR files in the
> shared path table at dump time. An entry can be added to the table and
> flagged as 'non-existing' if one is such a case. At runtime, you can
> check for that then. Please let me know if it's a bit vague, I can add
> more details.
As per our off-list discussion,  this could be done in a follow-up RFE. 
I'll ask you for more details when I file the RFE.

thanks,
Calvin
>
> Best regards,
> Jiangli
>> Call paths to the function:
>>
>> SharedPathsMiscInfo::check(jint, const char *, bool) : bool
>>       SharedPathsMiscInfo::check(bool) : bool
>>           ClassLoader::check_shared_paths_misc_info(void *, int, bool) : bool
>>               FileMapInfo::validate_header(bool) : bool
>>                   FileMapInfo::initialize(bool) : bool
>>                       DynamicArchive::map_impl(FileMapInfo *) : address
>>
>> MetaspaceShared::initialize_runtime_shared_and_meta_spaces() : void
>>> There are quite some additional SharedPathsMiscInfo cleanup and
>>> removal needed. If you want to do those in separate changeset, that is
>>> also okay. The cleanups will help make the CDS code more maintainable.
>> I agree that cleaning up SharedPathsMiscInfo is possible but more
>> thinking is required. So it should be done as a separate RFE/changeset.
>>
>>> - src/hotspot/os/posix/os_posix.cpp
>>>
>>> 1463   ret1 = os::stat(file1,&st1);
>>> 1464   ret2 = os::stat(file2,&st2);
>>> 1465   if (ret1<   0 || ret2<   0) {
>>> 1466     // one of the files is invalid. So they are not the same.
>>> 1467     is_same = false;
>>> 1468   } else if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) {
>>> 1469     // different files
>>> 1470     is_same = false;
>>> 1471   } else if (st1.st_dev == st2.st_dev&&   st1.st_ino == st2.st_ino) {
>>> 1472     // same files
>>> 1473     is_same = true;
>>> 1474   }
>>> 1475   return is_same;
>>>
>>> The 'is_same' is initialized to false, so the cases where the files
>>> are different do not need to be checked explicitly. We just need to
>>> make sure 'is_same' is set to true for the case where the two files
>>> are the same. So you can simplify and optimize the above a bit:
>>>
>>> if (os::stat(file1,&st1) != 0) {
>>>     return false;
>>> }
>>> if (os::stat(file2,&st2) != 0) {
>>>     return false;
>>> }
>>> if (st1.st_dev == st2.st_dev&&   st1.st_ino == st2.st_ino) {
>>>     is_same = true;
>>> }
>>> return is_same;
>> I've made the above change but checking the return status of os::stat as
>> what I had before because ::stat returns -1 on failure:
>>     if (os::stat(file1,&st1)<  0) {
>>       return false;
>>     }
>>
>>     if (os::stat(file2,&st2)<  0) {
>>       return false;
>>     }
>>
>>
>> So far I've changed:
>> M src/hotspot/os/posix/os_posix.cpp
>> M src/hotspot/share/memory/filemap.cpp
>> M src/hotspot/share/memory/filemap.hpp
>>
>> I'll do more testing before sending an updated webrevs.
>>
>> thanks,
>> Calvin
>>> Best regards,
>>>
>>> Jiangli