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

[Calvin Cheung]

On 3/7/19, 1:30 PM, Jiangli Zhou wrote:
> 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 
> <mailto: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!
Just checked with infra, the "submit repo testing" only performs tier1 
testing.
I also checked the tier1 definition and your new test should be run on 
the following platforms:
linux_x64_debug, macosx_x64_debug, windows_x64_debug

thanks,
Calvin
>
>
>     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/
>     <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
>