RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
jianglizhou at google.com
Mon Mar 11 01:41:40 UTC 2019
Here is the updated webrev:
- 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
- Reverted the changes in sharedPathsMiscInfo.cpp. Further testing
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.
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
> > files might have different names than the linked names. The VM crashes
> > '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
> > 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
> > 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/"
More information about the hotspot-runtime-dev