RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Fri Mar 15 17:35:31 UTC 2019
On Thu, Mar 14, 2019 at 9:52 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
> On 3/14/19 7:40 PM, Ioi Lam wrote:
> > I think the code is actually very simple. It's used in only 2 places:
> >
> > 1. For class loading log, print source=jrt:/java.base, etc, for those
> > classes loaded from the modules file by the boot class loader.
> >
> > 2. Decide that classpath_index = 0 during CDS dump, when the class is
> > loaded from the modules file.
> >
> > It's never intended to check classes that are not loaded by the boot
> > loader.
>
The place in ClassLoader::record_result that performs the is_modules_image
check does not take the notion of class loader. The underlying logic would
become different with your change. And that leads to my other question
that's related to your comments below about the source being 'jrt:'. In
which cases a class have the modules path string as the source? And in
which cases a class has the 'jrt:' protocol as the source? Can you please
check with Lois and Harold on these different cases?
The risk tolerance for backports is lower. A conserve fix without altering
the underlying logic too much probably is preferred in that case. If a two
step process addresses it better then it should be done. However, it
doesn't seem to worth continuing to spam the others in the mailing list to
resolve our different opinions. So I will reassign the bug to you and we
can resolve this issue efficiently (the bug report for this issue is a more
suitable place for all these technical discussions, it also helps
tracking).
Thanks,
Jiangli
> >
> >
> > On 3/14/19 6:00 PM, Jiangli Zhou wrote:
> >>
> >> On Thu, Mar 14, 2019 at 11:01 AM Ioi Lam <ioi.lam at oracle.com
> >> <mailto:ioi.lam at oracle.com>> wrote:
> >>
> >> Look at jvm_define_class_common in jvm.cpp. ClassFileStream can be
> >> stack allocated, without using the 'new' operator.
> >>
> >>
> http://closedjdk.us.oracle.com/jdk/closed-sandbox/open/file/5d095f4b2164/src/hotspot/share/prims/jvm.cpp#l944
> >>
> >>
> >> This could be solved by looking up the existing ClassPathEntry (and
> >> creating a new one if no existing one) from a hashtable. However, I
> >> realized the issue was it would still be relying on the comparison of
> >> the canonical path string (the 'name' argument) with the
> >> ClassPathEntry->name(). I also took a quick look (not a review) of
> >> your change. It appears it always assumes the class is not from the
> >> modules image in the jvm_define_class_common call path, since
> >> 'from_modules_image' defaults to false.
> >
> > If the name from_modules_image is not appropriate, how about
> > from_modules_image_by_boot_loader.
> >
> >> Application module classes can come from the runtime modules image,
> >> and the assumption can be problematic.
> >>
> I forgot to mention -- if the class come from the Java code via
> jvm_define_class_common(), the source is already set to "jrt://xxx.yyy".
> In this case, the source will never be the JAVA_HOME/lib/modules.
>
> You can experiment with
>
> javac -J-verbose -help
>
> Thanks
>
> - Ioi
>
>
>
> >> Pardon my ignorance, however I think this discussion does not need to
> >> be part of the code review of this bug fix and should the a follow up
> >> action. It deserves more investigations to cover all cases, including
> >> the edge cases that you have and those edge cases are not the ones
> >> this fix is trying to address.
> >>
> > The whole point of have a public review is to find a correct fix.
> >
> > If we can fix it correctly in 1 step with a simple fix, why should we
> > do it in 2 steps?
> >
> > We already have several people looking at this bug, and we have all
> > been confused by is_modules_image(). If we check in a partial fix now
> > by keeping the is_modules_image() API, we will all be confused again
> > when we fix it properly again.
> >
> > Fixing stuff in the JDK is not a light process, so 1 step is better
> > than 2.
> >
> > Thanks
> >
> > - Ioi
> >
> >
> >> Thanks,
> >> Jiangli
> >>
> >>
> >>
> >>
> >> Thanks
> >> - Ioi
> >>
> >> On 3/14/19 10:44 AM, Jiangli Zhou wrote:
> >>>
> >>> On Thu, Mar 14, 2019 at 10:26 AM Ioi Lam <ioi.lam at oracle.com
> >>> <mailto:ioi.lam at oracle.com>> wrote:
> >>>
> >>> 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.
> >>>
> >>>
> >>> Grep tells me that all the places creating a ClassFileStream with
> >>> a path string have known ClassPathEntry. Maybe I'm missing
> >>> something here? With ClassFileStream::_source becoming a
> >>> ClassPathEntry*, then all the is_modules_image(const char*) call
> >>> sites can use the ClassPathEntry::is_modules_image() API without
> >>> the path string comparison.
> >>>
> >>> Thanks,
> >>> Jiangli
> >>>
> >>>
> >>> 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