RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file

[Jiangli Zhou]

Hi Calvin,

Thanks a lot for the review! Please see some comments below.

On Thu, Mar 7, 2019 at 9:01 AM Calvin Cheung <calvin.cheung at oracle.com>
wrote:

> Hi Jiangli,
>
> Thanks for filing and fixing the bug.
>

You're welcome. ;)


>
> Just couple of suggestions:
>
> classLoader.cpp
>
> Currently, os::file_separator() contains only 1 character. Consider
> using strrchr instead of a while loop?
> You can keep the while loop in case in the len of os::file_separator is >
> 1.
>
> Something like the following:
> void ClassLoader::init_modules_image_name(const char* modules_path) {
>    // skip directory names
>    const char *p1, *p2;
>    p1 = modules_path;
>    int len = (int)strlen(os::file_separator());
>    if (len == 1) {
>      p2 = strrchr(modules_path, os::file_separator()[0]);
>      if (p2 != NULL) {
>        p1 = p2 + len;
>      }
>    } else {
>      while ((p2 = strstr(p1, os::file_separator())) != NULL) {
>        p1 = p2 + len;
>      }
>    }
>    _modules_image_name = os::strdup(p1);
> }
>

The call path is executed only once in this particular case. Using strrchr
like above sounds ok to me if there is no objection from others about
over-optimization. It could be useful to also extract it into a utility
function later in a separate patch. I think we have other places in the VM
also need to skip file separators when processing paths.


> ModulesSymLink.java
>
> 29  * @requires (os.family == "linux")
>
> I think the test should be applicable to the Macosx and the Solaris
> platforms too.
> So perhaps just excluding windows:
>          @requires (os.family != "windows")
>

Not sure how common the particular symbolic link usage is for non-linux
platforms. So it's hard to tell how valuable it is to execute this test
case for non-linux platforms, but I have no objection for enabling the test
on non-Windows in the main-line testing. Could you please check with
someone  with infra? Thanks!


>
> 72         String modules_prefix ...
>
> This variable is unused.
>

Good catch! Will remove.

Thanks and regard,

Jiangli


>
> thanks,
> Calvin
>
> On 3/6/19, 3:20 PM, Jiangli Zhou wrote:
> > Please review the following fix for 8220095.
> >
> > webrev: http://cr.openjdk.java.net/~jiangli/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
>