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

[Calvin Cheung]

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.

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?

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