[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]

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

>
>
> - 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.

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