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 18:01:36 UTC 2019
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
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