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

[Jiangli Zhou]

Hi Lois,

Thanks a lot for the review! Will incorporate all your suggestions and
update the webrev.

Thanks and regards,
Jiangli

On Thu, Mar 7, 2019 at 5:11 AM Lois Foltan <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/
> > 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
>
> -
>