[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
Jiangli Zhou
jianglizhou at google.com
Thu Jun 27 00:38:10 UTC 2019
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.
> >
> >
> > - 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.
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
More information about the hotspot-runtime-dev
mailing list