RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Thu Mar 14 17:44:04 UTC 2019
On Thu, Mar 14, 2019 at 10:26 AM Ioi Lam <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> 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> 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> 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> 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> 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>
>>>>>> 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> 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> 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>
>>>>>>>>> >> 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>> 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