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

[Ioi Lam]

On 3/11/19 7:30 PM, Ioi Lam wrote:
> 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!
>

BTW, If you have a symlink JAVA_HOME/lib/modules -> JAVA_HOME/lib/r, 
then your new version of is_modules_image will return true for any jar 
file.

See 
http://hg.openjdk.java.net/jdk/jdk/file/f984aca565c1/src/hotspot/share/oops/instanceKlass.cpp#l3370

         if (ClassLoader::is_modules_image(cfs->source())) {
           info_stream.print(" source: jrt:/%s", module_name);
         }

This will cause all classes loaded from modular JAR files be incorrectly 
printed as "jrt://"

Even if you also use the last "/" path separator, it's still not safe to 
use the filename alone to determine whether a file is indeed the modules 
image file. I.e., what if you have JAVA_HOME/lib/modules ->  
JAVA_HOME/lib/test.jar?


Also, I think we can have a version that's simpler than what I proposed 
below:

   + We can have at most a single ClassPathImageEntry.
   + All calls to ClassLoader::is_modules_image() are made after the 
ClassPathImageEntry is created

so:

   static bool is_modules_image(const char* name) {
     if (ClassPathImageEntry::singleton() != NULL) {
        return strcmp(name, ClassPathImageEntry::singleton()->name()) == 0);
     } else {
        assert(_exploded_entries != NULL, "must be running with exploded 
modules");
        return false;
     }
   }


This function should be changed to always return true

   bool ClassPathImageEntry::is_modules_image() const {
-   return ClassLoader::is_modules_image(name());
+   return true;
   }


To make sure we don't call is_modules_image before ClassPathImageEntry 
is created, we could do

   static bool is_modules_image(const char* name) {
     DEBUG_ONLY(_is_modules_image_called = true;)
     ...
   }


   ClassPathImageEntry::ClassPathImageEntry() {
     assert(_singleton == NULL, "...");
     assert(!_is_modules_image_called, "Don't call 
ClassLoader::is_modules_image() before ClassPathImageEntry is allocated");
     ...


Thanks
- Ioi


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