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

[Ioi Lam]

I think this patch should be further improved.

First of all, the original code is problematic:

   static bool is_modules_image(const char* name) { return 
string_ends_with(name, MODULES_IMAGE_NAME); }

So if we have a file called /foo/bar/xxxmodules, this function returns true!

It seems the patch simply makes the code more convoluted. Instead, 
shouldn't we just open the file and check the magic number?

If performance is important, we can use a cache, because we allow a 
single module image:

static bool is_modules_image(const char* name) {
   static const char* singleton_module_name = NULL;

   if (singleton_module_name != NULL) {
     return strcmp(name, singleton_module_name) == 0;
   }
   // open <name> and check magic number
   if (good) {
     singleton_module_name = os::strdup(name);
     return true;
   }
   return false;
}

Thanks
- Ioi

On 3/11/19 10:51 AM, Jiangli Zhou wrote:
> 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
>>>
>>>      -
>>>