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

[Jiangli Zhou]

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
> >
> >     -
> >
>