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