RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Ioi Lam
ioi.lam at oracle.com
Fri Mar 15 02:40:19 UTC 2019
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.
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.
>
> 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