RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file

Ioi Lam ioi.lam at oracle.com
Thu Mar 14 17:26:08 UTC 2019


What problem are you trying to solve by putting the ClassPathEntry into 
ClassFileStream?

ClassFileStream can be created without a ClassPathEntry. E.g., when the 
classfile data comes from Java code.

On 3/14/19 9:54 AM, Jiangli Zhou wrote:
> I was going to propose eliminating the usage of is_modules_image(const 
> char*) as well. I'm in the middle of refactoring ClassFileStream to 
> take the 'source' as a * instead of a plain char*. I think that's a 
> more elegant approach. However, this issue needs to be addressed in 
> JDK 11 and 12 also. So a simpler fix should be applied first and 
> backported to 11 & 12. The webrev.02 or webrev.03 can be used for the 
> initial fix and backporting. Then we can be unblocked by this symlink 
> issue.
>
> The elimination of  is_modules_image(const char*) and refactoring work 
> can be done with a different RFE. If you want to take over from there, 
> that is ok with me.
>
> Thanks,
> Jiangli
>
> On Wed, Mar 13, 2019 at 11:46 PM Ioi Lam <ioi.lam at oracle.com 
> <mailto:ioi.lam at oracle.com>> wrote:
>
>     I have a different idea ....
>
>     I think we are all confused by is_modules_image(const char* name).
>     It's a poorly designed interface -- it's not clear what it's
>     intended for, and what its capabilities are (Can it handle
>     case-insensitivity? Can it handle non-canonical names? Should it
>     do a magic number check to see if the file is indeed a jimage file?).
>
>     I think we should remove this function, otherwise it might be
>     abused in the future and cause more problems.
>
>     We should replace it with what we actually need. I.e., check if a
>     ClassFileStream was created from the jimage.
>
>     I have written a simple fix and it passes all CDS tests,
>     tier1/tier2, as well as Jiangli's new test
>
>     http://cr.openjdk.java.net/~iklam/jdk13/8220095_remove_is_modules_image.v00/
>
>     Any thoughts?
>
>     Thanks
>     - Ioi
>
>
>
>     On 3/13/19 6:24 PM, Jiangli Zhou wrote:
>>
>>
>>     On Wed, Mar 13, 2019 at 1:54 PM Ioi Lam <ioi.lam at oracle.com
>>     <mailto:ioi.lam at oracle.com>> wrote:
>>
>>
>>         On 3/13/2019 1:45 PM, Jiangli Zhou wrote:
>>>         On Wed, Mar 13, 2019 at 12:21 PM Ioi Lam <ioi.lam at oracle.com
>>>         <mailto:ioi.lam at oracle.com>> wrote:
>>>
>>>
>>>             On 3/13/2019 12:03 PM, Jiangli Zhou wrote:
>>>>
>>>>
>>>>             On Wed, Mar 13, 2019 at 11:53 AM Ioi Lam
>>>>             <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>>>
>>>>
>>>>                 On 3/13/2019 11:41 AM, Jiangli Zhou wrote:
>>>>>
>>>>>
>>>>>                 On Wed, Mar 13, 2019 at 11:03 AM Ioi Lam
>>>>>                 <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>>
>>>>>                 wrote:
>>>>>
>>>>>                     Hi Jiangli,
>>>>>
>>>>>                     Thanks for making the changes. This makes the
>>>>>                     code much simpler and fixes the original
>>>>>                     problem with the improper use of
>>>>>                     MODULES_IMAGE_NAME.
>>>>>
>>>>>                     [1] Naming and comments:
>>>>>
>>>>>                     modules_image_identity: how about
>>>>>                     modules_image_canonical_path, so we know what
>>>>>                     it actually is?
>>>>>
>>>>>
>>>>>                 The modules_image_identity naming appears better
>>>>>                 as the canonical path is used as an identity here.
>>>>>                 It tells the intention of the variable.
>>>>>
>>>>>
>>>>>                     is_modules_image(const char* name) -->
>>>>>                     is_modules_image(const char* canonical_path)
>>>>>
>>>>>
>>>>>                 I considered to change the name of the argument
>>>>>                 but decided against it. The sources of the
>>>>>                 argument can come from a ClassPathEntry name in
>>>>>                 some of the places. It creates confusion without
>>>>>                 also change the other existing code. A
>>>>>                 separate exercise can be used to clean up the
>>>>>                 existing code.
>>>>>
>>>>>
>>>>>                     class ClassPathImageEntry ... {
>>>>>                     + // returns canonical path of
>>>>>                     <JAVA_HOME>/lib/<MODULES_IMAGE_NAME>
>>>>>                       const char* name() const { return _name ==
>>>>>                     NULL ? "" : _name; }
>>>>>
>>>>>
>>>>>                     [2] 142 const char*
>>>>>                     ClassLoader::_modules_image_identity =
>>>>>                     MODULES_IMAGE_NAME;
>>>>>
>>>>>                      ... but your version of is_modules_image()
>>>>>                     assumes the uninitialized value is NULL.
>>>>>
>>>>>
>>>>>                 Changed to initialize it to NULL.
>>>>>
>>>>>
>>>>>                     [3] For the following code:
>>>>>
>>>>>                      452   static bool is_modules_image(const
>>>>>                     char* name) {
>>>>>                      453     if (Arguments::has_jimage()) {
>>>>>                      454 assert(modules_image_identity() != NULL,
>>>>>                     "must be set");
>>>>>                      455 return strcmp(name,
>>>>>                     modules_image_identity()) == 0;
>>>>>                      456     }
>>>>>                      457 return false;
>>>>>                      458   }
>>>>>
>>>>>                     Instead of maintaining a copy of the jimage's
>>>>>                     name in _modules_image_identity, how about
>>>>>
>>>>>                     const char* modules_image_identity() {
>>>>>                     assert(get_jrt_entry() != NULL, "must be inited");
>>>>>                        return get_jrt_entry()->name();
>>>>>                     }
>>>>>
>>>>>                     Also, I think in most cases, the name is
>>>>>                     exactly the same pointer as
>>>>>                     get_jrt_entry()->name(), so line 455 can be
>>>>>                     changed to
>>>>>
>>>>>                         const char* id = modules_image_identity();
>>>>>                         return name == id || strcmp(name, id) == 0;
>>>>>
>>>>>                     Also, I think you should change
>>>>>                     ClassPathImageEntry::is_modules_image() to
>>>>>                     simply return true. Otherwise it's just
>>>>>                     comparing its name with its name .... If you
>>>>>                     do this, then you can add another assert to my
>>>>>                     version of modules_image_identity(), without
>>>>>                     worrying above infinite recursion:
>>>>>
>>>>>                     assert(get_jrt_entry()->is_modules_image(),
>>>>>                     "must be");
>>>>>
>>>>>
>>>>>                 I'm hesitate to use ClassPathImageEntry name
>>>>>                 directly because it may not necessary to be full
>>>>>                 canonical path. Using a separate variable for
>>>>>                 modules identity seems to be more explicit and
>>>>>                 less error-prone in the future.
>>>>
>>>>                 But your code essentially just makes a copy of
>>>>                 get_jrt_entry()->name()? How does that address your
>>>>                 concern?
>>>>
>>>>
>>>>             True, it makes an assumption about the existing
>>>>             implementation, which might not be future-proof. One
>>>>             solution is to change create_class_path_entry() to give
>>>>             back  the canonical path explicitly.
>>>>
>>>             Our problem is not strictly limited to canonical paths.
>>>             All of the callers that expect a positive return from
>>>             is_modules_image() get their input from
>>>             get_jrt_entry()->name(). For example, see
>>>             ClassLoader::load_class(). The caller wants to know "is
>>>             the src, supplied in a ClassFileStream, the same as
>>>             get_jrt_entry()".
>>>
>>>             -> Just grep with 'is_modules_image[(][^)]'. There are
>>>             very few callers.
>>>
>>>             The problem is get_jrt_entry()->name() may be subject to
>>>             transformation, so that it's not necessarily
>>>             JAVA_HOME/lib/modules. In this particular bug, the
>>>             transformation is pathname canonicalization. It's
>>>             possible to use other transformations. E.g., change it
>>>             to "?modules?".
>>>
>>>             So the check we really want to do is: strcmp(name,
>>>             get_jrt_entry()->name());
>>>
>>>
>>>         There are mixed sources of the path strings when calling
>>>         into is_modules_image(const char*). The
>>>         ClassFileStream::_source is one and ClassPathEntry->name()
>>>         is another case. The ClassFileStream::_source is the
>>>         canonical path of the class file stream.
>>
>>
>>         ClassLoader::load_class() {
>>
>>         ...
>>          stream = _jrt_entry->open_stream(file_name, CHECK_NULL);
>>         ...
>>
>>         ClassFileStream* ClassPathImageEntry::open_stream(const char*
>>         name, TRAPS) {
>>         ...
>>             return new ClassFileStream((u1*)data,
>>                                        (int)size,
>>                                        _name,
>>         ClassFileStream::verify);
>>
>>         So the ClassFileStream contains the same string as in the
>>         _jrt_entry?
>>
>>         Where do you see this "_source is the canonical path of the
>>         class file stream" happening?
>>
>>
>>     You are right, the ClassFileStream created for the runtime image
>>     uses ClassPathImageEntry::_name as the _source. So there is only
>>     a single source of the path string, which is jrt_entry '_name'.
>>     Using that within the check probably is ok then.
>>
>>     Thanks,
>>     Jiangli
>>
>>
>>
>>>         If the ClassPathEntry->name() changes as you describe above
>>>         (which is also the source of my hesitation), then using
>>>         get_jrt_entry()->name() in the check would cause an issue.
>>>         That's also the reason why I didn't think a simple strcmp
>>>         was a good idea.
>>>
>>>         If you are pushing to use get_jrt_entry()->name() and do a
>>>         simple strcmp, then it needs to guarantee
>>>         get_jrt_entry()->name() will always give the full canonical
>>>         path. Not sure if a comment with a warning
>>>         in ClassPathImageEntry class is sufficient?
>>>
>>>         Thanks,
>>>         Jiangli
>>>
>>>             Thanks
>>>
>>>
>>>
>>>>             Thanks,
>>>>             Jiangli
>>>>
>>>>
>>>>
>>>>>
>>>>>                 Thanks,
>>>>>                 Jiangli
>>>>>
>>>>>
>>>>>                     Thanks
>>>>>                     - Ioi
>>>>>
>>>>>                     On 3/13/19 10:22 AM, Jiangli Zhou wrote:
>>>>>>
>>>>>>
>>>>>>                     On Tue, Mar 12, 2019 at 4:42 PM Jiangli Zhou
>>>>>>                     <jianglizhou at google.com
>>>>>>                     <mailto:jianglizhou at google.com>> wrote:
>>>>>>
>>>>>>                         One of my early experimental revision
>>>>>>                         actually simply used strcmp to compare
>>>>>>                         the full path and to also recorded the
>>>>>>                         full path for the canonical modules name.
>>>>>>
>>>>>>                         I decide to not go with that because
>>>>>>                         there is no guarantee that the path given
>>>>>>                         to is_modules_image check is in canonical
>>>>>>                         form and it diverts from the existing
>>>>>>                         assumptions about the usages of this API
>>>>>>                         (checking the file name only).
>>>>>>                         The "/dev/fd/0" example you have is not a
>>>>>>                         direct result of the current fix but is
>>>>>>                         an inherent issue of the modules check.
>>>>>>                         Any file with the same designated modules
>>>>>>                         name would pass the check. If we want to
>>>>>>                         safely use the full path comparison, we
>>>>>>                         need to closely exam all the call sites
>>>>>>                         and make sure all the places use
>>>>>>                         canonical paths. I'll some analysis on that.
>>>>>>
>>>>>>
>>>>>>                     I've double checked all existing call sites
>>>>>>                     of is_modules_image(const char*). No extra
>>>>>>                     processing is done (e.g. extracting the file
>>>>>>                     name) on the path strings when passing to
>>>>>>                     is_modules_image. Using the full string
>>>>>>                     comparison appears to be safe so I have no
>>>>>>                     objection. I've reverted my change to record
>>>>>>                     the complete canonical path as the modules
>>>>>>                     identity and changed to do full path string
>>>>>>                     comparison:
>>>>>>
>>>>>>                     http://cr.openjdk.java.net/~jiangli/8220095/webrev.02/
>>>>>>
>>>>>>                     Local testing looks good. I've also submitted
>>>>>>                     a submit-repo testing. Please help rerun
>>>>>>                     tier2, tier3 and maybe some of the higher
>>>>>>                     tier tests due to the semantics change of
>>>>>>                     is_modules_image.
>>>>>>
>>>>>>                     Thanks,
>>>>>>                     Jiangli
>>>>>>
>>>>>>
>>>>>>                         Thanks,
>>>>>>                         Jiangli
>>>>>>
>>>>>>                         On Tue, Mar 12, 2019 at 4:13 PM Ioi Lam
>>>>>>                         <ioi.lam at oracle.com
>>>>>>                         <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>
>>>>>>                             This seems more complicated that it
>>>>>>                             needs to be.
>>>>>>
>>>>>>                             + The VM can have at most one modules
>>>>>>                             file.
>>>>>>
>>>>>>                             + The only reason the VM calls
>>>>>>                             is_modules_image(p) is to check
>>>>>>                             whether p, a full path, is THE
>>>>>>                             modules file. In all cases, p is
>>>>>>                             actually derived from the singleton
>>>>>>                             ClassPathImageEntry::name(). It's
>>>>>>                             just that in some cases, such as in
>>>>>>                             the logging code, we have lost the
>>>>>>                             pointer to the ClassPathImageEntry,
>>>>>>                             and just have the full path.
>>>>>>
>>>>>>                             + On line 722 of
>>>>>>                             http://cr.openjdk.java.net/~jiangli/8220095/webrev.01/src/hotspot/share/classfile/classLoader.cpp.sdiff.html,
>>>>>>                             you already have a full path of THE
>>>>>>                             modules file.
>>>>>>
>>>>>>                             Why can't you just do a strcmp of the
>>>>>>                             full path? That's much simpler, and
>>>>>>                             more importantly, correct. Your
>>>>>>                             version will return true for
>>>>>>                             "/dev/fd/0" for the test case
>>>>>>                             described in the bug report. That
>>>>>>                             doesn't seem to be correct.
>>>>>>
>>>>>>
>>>>>>                             Thanks
>>>>>>                             - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>>                             On 3/12/19 9:10 AM, Jiangli Zhou wrote:
>>>>>>>                             Hi Ioi,
>>>>>>>
>>>>>>>                             Thanks for looking into this also.
>>>>>>>
>>>>>>>                             On Mon, Mar 11, 2019 at 8:19 PM Ioi
>>>>>>>                             Lam <ioi.lam at oracle.com
>>>>>>>                             <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                                 On 3/11/19 7:30 PM, Ioi Lam wrote:
>>>>>>>                                 > I think this patch should be
>>>>>>>                                 further improved.
>>>>>>>                                 >
>>>>>>>                                 > First of all, the original
>>>>>>>                                 code is problematic:
>>>>>>>                                 >
>>>>>>>                                 >   static bool
>>>>>>>                                 is_modules_image(const char*
>>>>>>>                                 name) { return
>>>>>>>                                 > string_ends_with(name,
>>>>>>>                                 MODULES_IMAGE_NAME); }
>>>>>>>                                 >
>>>>>>>                                 > So if we have a file called
>>>>>>>                                 /foo/bar/xxxmodules, this
>>>>>>>                                 function returns
>>>>>>>                                 > true!
>>>>>>>                                 >
>>>>>>>
>>>>>>>                                 BTW, If you have a symlink
>>>>>>>                                 JAVA_HOME/lib/modules ->
>>>>>>>                                 JAVA_HOME/lib/r,
>>>>>>>                                 then your new version of
>>>>>>>                                 is_modules_image will return
>>>>>>>                                 true for any jar
>>>>>>>                                 file.
>>>>>>>
>>>>>>>                                 See
>>>>>>>                                 http://hg.openjdk.java.net/jdk/jdk/file/f984aca565c1/src/hotspot/share/oops/instanceKlass.cpp#l3370
>>>>>>>
>>>>>>>                                          if
>>>>>>>                                 (ClassLoader::is_modules_image(cfs->source()))
>>>>>>>                                 {
>>>>>>>                                 info_stream.print(" source:
>>>>>>>                                 jrt:/%s", module_name);
>>>>>>>                                          }
>>>>>>>
>>>>>>>                                 This will cause all classes
>>>>>>>                                 loaded from modular JAR files be
>>>>>>>                                 incorrectly
>>>>>>>                                 printed as "jrt://"
>>>>>>>
>>>>>>>                                 Even if you also use the last
>>>>>>>                                 "/" path separator, it's still
>>>>>>>                                 not safe to
>>>>>>>                                 use the filename alone to
>>>>>>>                                 determine whether a file is
>>>>>>>                                 indeed the modules
>>>>>>>                                 image file. I.e., what if you
>>>>>>>                                 have JAVA_HOME/lib/modules ->
>>>>>>>                                 JAVA_HOME/lib/test.jar?
>>>>>>>
>>>>>>>
>>>>>>>                             The current bug addresses the
>>>>>>>                             practical issue when the modules is
>>>>>>>                             a sym link to a unique content
>>>>>>>                             identifier. Using modules as a link
>>>>>>>                             to any arbitrary file is not
>>>>>>>                             addressed by this bug. It's in the
>>>>>>>                             same area but a different issue.
>>>>>>>
>>>>>>>                             If lib/modules ->lib/test.jar, or
>>>>>>>                             lib/modules -> ../ (or any other
>>>>>>>                             arbitrary file/dir) is done, the VM
>>>>>>>                             fails differently. This is the same
>>>>>>>                             as if you replace libjvm.so with
>>>>>>>                             another random file.
>>>>>>>
>>>>>>>                             The canonical modules name is
>>>>>>>                             obtained from the actual modules
>>>>>>>                             image during setup_boot_search_path,
>>>>>>>                             so it's guaranteed to refer to the
>>>>>>>                             correct identifier. Checking the
>>>>>>>                             file name after '/' is safe.
>>>>>>>
>>>>>>>                             The substring issue that you pointed
>>>>>>>                             out has not been observed, as that's
>>>>>>>                             prevented by the naming method of
>>>>>>>                             the content identifiers. The
>>>>>>>                             substring issue however can be
>>>>>>>                             solved relatively inexpensively by
>>>>>>>                             making sure the check covers the
>>>>>>>                             entire file name after '/'.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                                 Also, I think we can have a
>>>>>>>                                 version that's simpler than what
>>>>>>>                                 I proposed
>>>>>>>                                 below:
>>>>>>>
>>>>>>>                                    + We can have at most a
>>>>>>>                                 single ClassPathImageEntry.
>>>>>>>                                    + All calls to
>>>>>>>                                 ClassLoader::is_modules_image()
>>>>>>>                                 are made after the
>>>>>>>                                 ClassPathImageEntry is created
>>>>>>>
>>>>>>>                                 so:
>>>>>>>
>>>>>>>                                    static bool
>>>>>>>                                 is_modules_image(const char* name) {
>>>>>>>                                      if
>>>>>>>                                 (ClassPathImageEntry::singleton()
>>>>>>>                                 != NULL) {
>>>>>>>                                         return strcmp(name,
>>>>>>>                                 ClassPathImageEntry::singleton()->name())
>>>>>>>                                 == 0);
>>>>>>>                                      } else {
>>>>>>>                                 assert(_exploded_entries !=
>>>>>>>                                 NULL, "must be running with
>>>>>>>                                 exploded
>>>>>>>                                 modules");
>>>>>>>                                         return false;
>>>>>>>                                      }
>>>>>>>                                    }
>>>>>>>
>>>>>>>
>>>>>>>                             The argument passed can be a path to
>>>>>>>                             the modules, so strcmp cannot be
>>>>>>>                             simply used for 'name'.
>>>>>>>
>>>>>>>                             static bool is_modules_image(const
>>>>>>>                             char* name) {
>>>>>>>
>>>>>>>                             if (Arguments::has_jimage()) {
>>>>>>>
>>>>>>>                             assert(_modules_image_name != NULL,
>>>>>>>                             "must be set");
>>>>>>>
>>>>>>>                             const char* p = skip_directories(name);
>>>>>>>
>>>>>>>                             return strcmp(p,
>>>>>>>                             _modules_image_name) == 0;
>>>>>>>
>>>>>>>                             }
>>>>>>>
>>>>>>>                             return false;
>>>>>>>
>>>>>>>                             }
>>>>>>>
>>>>>>>
>>>>>>>                             I've revised the is_modules_image()
>>>>>>>                             to also cover the substring issue.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                                 This function should be changed
>>>>>>>                                 to always return true
>>>>>>>
>>>>>>>                                    bool
>>>>>>>                                 ClassPathImageEntry::is_modules_image()
>>>>>>>                                 const {
>>>>>>>                                 -   return
>>>>>>>                                 ClassLoader::is_modules_image(name());
>>>>>>>                                 +   return true;
>>>>>>>                                    }
>>>>>>>
>>>>>>>
>>>>>>>                             Leaving
>>>>>>>                             ClassPathImageEntry::is_modules_image()
>>>>>>>                             without changing is safer, so it can
>>>>>>>                             make sure the ClassPathImageEntry
>>>>>>>                             name agrees with the canonical
>>>>>>>                             modules name.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                                 To make sure we don't call
>>>>>>>                                 is_modules_image before
>>>>>>>                                 ClassPathImageEntry
>>>>>>>                                 is created, we could do
>>>>>>>
>>>>>>>                                    static bool
>>>>>>>                                 is_modules_image(const char* name) {
>>>>>>>                                 DEBUG_ONLY(_is_modules_image_called
>>>>>>>                                 = true;)
>>>>>>>                                      ...
>>>>>>>                                    }
>>>>>>>
>>>>>>>
>>>>>>>                             The 'assert(_modules_image_name !=
>>>>>>>                             NULL, "must be set")' check makes
>>>>>>>                             sure is_modules_image(const char*)
>>>>>>>                             is not called
>>>>>>>                             before setup_boot_search_path. There
>>>>>>>                             is no need to add additional flag.
>>>>>>>
>>>>>>>                             Thanks,
>>>>>>>                             Jiangli
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                                 ClassPathImageEntry::ClassPathImageEntry()
>>>>>>>                                 {
>>>>>>>                                 assert(_singleton == NULL, "...");
>>>>>>>                                 assert(!_is_modules_image_called,
>>>>>>>                                 "Don't call
>>>>>>>                                 ClassLoader::is_modules_image()
>>>>>>>                                 before ClassPathImageEntry is
>>>>>>>                                 allocated");
>>>>>>>                                      ...
>>>>>>>
>>>>>>>
>>>>>>>                                 Thanks
>>>>>>>                                 - Ioi
>>>>>>>
>>>>>>>
>>>>>>>                                 > It seems the patch simply
>>>>>>>                                 makes the code more convoluted.
>>>>>>>                                 Instead,
>>>>>>>                                 > shouldn't we just open the
>>>>>>>                                 file and check the magic number?
>>>>>>>                                 >
>>>>>>>                                 > If performance is important,
>>>>>>>                                 we can use a cache, because we
>>>>>>>                                 allow a
>>>>>>>                                 > single module image:
>>>>>>>                                 >
>>>>>>>                                 > static bool
>>>>>>>                                 is_modules_image(const char* name) {
>>>>>>>                                 >   static const char*
>>>>>>>                                 singleton_module_name = NULL;
>>>>>>>                                 >
>>>>>>>                                 >   if (singleton_module_name !=
>>>>>>>                                 NULL) {
>>>>>>>                                 > return strcmp(name,
>>>>>>>                                 singleton_module_name) == 0;
>>>>>>>                                 >   }
>>>>>>>                                 >   // open <name> and check
>>>>>>>                                 magic number
>>>>>>>                                 >   if (good) {
>>>>>>>                                 > singleton_module_name =
>>>>>>>                                 os::strdup(name);
>>>>>>>                                 > return true;
>>>>>>>                                 >   }
>>>>>>>                                 >   return false;
>>>>>>>                                 > }
>>>>>>>                                 >
>>>>>>>                                 > Thanks
>>>>>>>                                 > - Ioi
>>>>>>>                                 >
>>>>>>>                                 > On 3/11/19 10:51 AM, Jiangli
>>>>>>>                                 Zhou wrote:
>>>>>>>                                 >> On Mon, Mar 11, 2019 at 8:44
>>>>>>>                                 AM Calvin Cheung
>>>>>>>                                 <calvin.cheung at oracle.com
>>>>>>>                                 <mailto:calvin.cheung at oracle.com>>
>>>>>>>                                 >> wrote:
>>>>>>>                                 >>
>>>>>>>                                 >>> Hi Jiangli,
>>>>>>>                                 >>>
>>>>>>>                                 >>> The updated webrev looks good.
>>>>>>>                                 >>>
>>>>>>>                                 >>> On 3/10/19, 6:41 PM, Jiangli
>>>>>>>                                 Zhou wrote:
>>>>>>>                                 >>>> Here is the updated webrev:
>>>>>>>                                 >>>>
>>>>>>>                                 http://cr.openjdk.java.net/~jiangli/8220095/webrev.01/
>>>>>>>                                 >>>>
>>>>>>>                                 <http://cr.openjdk.java.net/%7Ejiangli/8220095/webrev.01/>.
>>>>>>>                                 >>>>
>>>>>>>                                 >>>> - Incorporated all
>>>>>>>                                 suggestions except the test
>>>>>>>                                 platforms for
>>>>>>>                                 >>>> ModulesSymLink.java. I
>>>>>>>                                 changed to requires linux, mac
>>>>>>>                                 and solaris
>>>>>>>                                 >>>> explicitly instead to avoid
>>>>>>>                                 any potential issue with other
>>>>>>>                                 >>>> non-mainline testing platforms.
>>>>>>>                                 >>>> - Reverted the changes in
>>>>>>>                                 sharedPathsMiscInfo.cpp. Further
>>>>>>>                                 testing
>>>>>>>                                 >>>> showed
>>>>>>>                                 SharedPathsMiscInfo::check()
>>>>>>>                                 were dealing the system boot path
>>>>>>>                                 >>>> string set up by
>>>>>>>                                 os::set_boot_path. Lois comments
>>>>>>>                                 reminded me to
>>>>>>>                                 >>>> double check with this. Thanks!
>>>>>>>                                 >>>>
>>>>>>>                                 >>>> Please help fire tier2 and
>>>>>>>                                 tier3 runs with mach5.
>>>>>>>                                 >>> I ran tier2 and tier3 tests
>>>>>>>                                 with your latest changes and saw
>>>>>>>                                 no test
>>>>>>>                                 >>> failures.
>>>>>>>                                 >>>
>>>>>>>                                 >> Great, thanks a lot!
>>>>>>>                                 >>
>>>>>>>                                 >>
>>>>>>>                                 >>> BTW, I was unable to
>>>>>>>                                 download the jdk.patch - got
>>>>>>>                                 "403 - forbidden"
>>>>>>>                                 >>> error.
>>>>>>>                                 >>> Same error was seen with
>>>>>>>                                 individual patch files.
>>>>>>>                                 >>> I used wget to get all the
>>>>>>>                                 raw files into my repo and then
>>>>>>>                                 submitted
>>>>>>>                                 >>> the
>>>>>>>                                 >>> build and test job.
>>>>>>>                                 >>> Could you check the file
>>>>>>>                                 permissions?
>>>>>>>                                 >>>
>>>>>>>                                 >> Yes, that's the file
>>>>>>>                                 permission issue. I fixed the
>>>>>>>                                 permission for the
>>>>>>>                                 >> jdk.patch.
>>>>>>>                                 >>
>>>>>>>                                 >> Thanks!
>>>>>>>                                 >> Jiangli
>>>>>>>                                 >>
>>>>>>>                                 >>
>>>>>>>                                 >>> thanks,
>>>>>>>                                 >>> Calvin
>>>>>>>                                 >>>> Thanks!
>>>>>>>                                 >>>> Jiangli
>>>>>>>                                 >>>>
>>>>>>>                                 >>>> On Thu, Mar 7, 2019 at 5:11
>>>>>>>                                 AM Lois Foltan
>>>>>>>                                 <lois.foltan at oracle.com
>>>>>>>                                 <mailto:lois.foltan at oracle.com>
>>>>>>>                                 >>>>
>>>>>>>                                 <mailto:lois.foltan at oracle.com
>>>>>>>                                 <mailto:lois.foltan at oracle.com>>>
>>>>>>>                                 wrote:
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      On 3/6/2019 6:20 PM,
>>>>>>>                                 Jiangli Zhou wrote:
>>>>>>>                                 >>>>      > Please review the
>>>>>>>                                 following fix for 8220095.
>>>>>>>                                 >>>>      >
>>>>>>>                                 >>>>      > webrev:
>>>>>>>                                 http://cr.openjdk.java.net/~jiangli/8220095/webrev.00/
>>>>>>>                                 >>>>
>>>>>>>                                 <http://cr.openjdk.java.net/%7Ejiangli/8220095/webrev.00/>
>>>>>>>                                 >>>>      > bug:
>>>>>>>                                 https://bugs.openjdk.java.net/browse/JDK-8220095
>>>>>>>                                 >>>>      >
>>>>>>>                                 >>>>      > Symbolic links may
>>>>>>>                                 be used commonly in some cloud
>>>>>>>                                 environments.
>>>>>>>                                 >>>>      The target
>>>>>>>                                 >>>>      > files might have
>>>>>>>                                 different names than the linked
>>>>>>>                                 names. The VM
>>>>>>>                                 >>>>      crashes when
>>>>>>>                                 >>>>      > 'lib/modules' links
>>>>>>>                                 to a target file with a
>>>>>>>                                 different name, for
>>>>>>>                                 >>>>      example,
>>>>>>>                                 >>>>      > lib/modules -> lib/0.
>>>>>>>                                 >>>>      >
>>>>>>>                                 >>>>      > The usage of the
>>>>>>>                                 hard-coded MODULES_IMAGE_NAME
>>>>>>>                                 (used by
>>>>>>>                                 >>>>      >
>>>>>>>                                 ClassLoader::is_modules_image(const
>>>>>>>                                 char*)) can become
>>>>>>>                                 >>>>      problematic in this
>>>>>>>                                 >>>>      > case and leads to
>>>>>>>                                 assertion failures and crashes
>>>>>>>                                 in this
>>>>>>>                                 >>>> case. The
>>>>>>>                                 >>>>      > is_modules_image()
>>>>>>>                                 >>>>      > checks always fail
>>>>>>>                                 even the actual file is the
>>>>>>>                                 runtime image
>>>>>>>                                 >>>>      because the
>>>>>>>                                 >>>>      > canonical paths
>>>>>>>                                 (which might not end with
>>>>>>>                                 'modules') are passed
>>>>>>>                                 >>>>      in for the
>>>>>>>                                 >>>>      > check. The fix is to
>>>>>>>                                 obtain the real name from the
>>>>>>>                                 canonical
>>>>>>>                                 >>>>      path early
>>>>>>>                                 >>>>      > during
>>>>>>>                                 initialization and use that for
>>>>>>>                                 the is_modules_image()
>>>>>>>                                 >>> check.
>>>>>>>                                 >>>>      >
>>>>>>>                                 >>>>      > Tested with submit
>>>>>>>                                 repo testing, tier1, tier2, and
>>>>>>>                                 hotspot
>>>>>>>                                 >>>>      runtime tests
>>>>>>>                                 >>>>      > locally.
>>>>>>>                                 >>>>      >
>>>>>>>                                 >>>>      > Thanks!
>>>>>>>                                 >>>>      > Jiangli
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      Hi Jiangli,
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      This change looks
>>>>>>>                                 good!  A couple of minor comments:
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      -  classLoader.hpp
>>>>>>>                                 >>>>           line #39 - I
>>>>>>>                                 think you should expand on the
>>>>>>>                                 comment and add
>>>>>>>                                 >>>>      why in
>>>>>>>                                 >>>>      the case of the
>>>>>>>                                 canonical path name
>>>>>>>                                 MODULES_IMAGE_NAME can not be
>>>>>>>                                 >>>>      used
>>>>>>>                                 >>>>      as-is but if one is
>>>>>>>                                 dealing with the system boot
>>>>>>>                                 path string set
>>>>>>>                                 >>>>      up by
>>>>>>>                                 >>>>      os::set_boot_path,
>>>>>>>                                 than MODULES_IMAGE_NAME should
>>>>>>>                                 be used.
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      - classLoader.cpp
>>>>>>>                                 >>>>          line #702 - Can
>>>>>>>                                 you add the same explanation
>>>>>>>                                 comment as in
>>>>>>>                                 >>>>      classLoader.hpp ahead
>>>>>>>                                 of the new
>>>>>>>                                 >>>>
>>>>>>>                                 ClassLoader::init_modules_image_name()
>>>>>>>                                 >>>>      method?  That would be
>>>>>>>                                 helpful.
>>>>>>>                                 >>>>          line #710 - ahead
>>>>>>>                                 of setting _modules_image_name
>>>>>>>                                 please
>>>>>>>                                 >>>> consider
>>>>>>>                                 >>>>      adding an assert that
>>>>>>>                                 p1's length is greater than 0?
>>>>>>>                                 >>>>
>>>>>>>                                                             
>>>>>>>                                 Maybe there should there be an
>>>>>>>                                 error case
>>>>>>>                                 >>>>      test
>>>>>>>                                 >>>>      where "lib/modules ->
>>>>>>>                                 lib/"
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      Thanks,
>>>>>>>                                 >>>>      Lois
>>>>>>>                                 >>>>
>>>>>>>                                 >>>>      -
>>>>>>>                                 >>>>
>>>>>>>                                 >
>>>>>>>
>>>>>>
>>>>>
>



More information about the hotspot-runtime-dev mailing list