RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Thu Mar 14 01:24:08 UTC 2019
On Wed, Mar 13, 2019 at 1:54 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
> On 3/13/2019 1:45 PM, Jiangli Zhou wrote:
>
> On Wed, Mar 13, 2019 at 12:21 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>
>>
>> On 3/13/2019 12:03 PM, Jiangli Zhou wrote:
>>
>>
>>
>> On Wed, Mar 13, 2019 at 11:53 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>>
>>> On 3/13/2019 11:41 AM, Jiangli Zhou wrote:
>>>
>>>
>>>
>>> On Wed, Mar 13, 2019 at 11:03 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>>> Hi Jiangli,
>>>>
>>>> Thanks for making the changes. This makes the code much simpler and
>>>> fixes the original problem with the improper use of MODULES_IMAGE_NAME.
>>>>
>>>> [1] Naming and comments:
>>>>
>>>> modules_image_identity: how about modules_image_canonical_path, so we
>>>> know what it actually is?
>>>>
>>>
>>> The modules_image_identity naming appears better as the canonical path
>>> is used as an identity here. It tells the intention of the variable.
>>>
>>>
>>>>
>>>> is_modules_image(const char* name) --> is_modules_image(const char*
>>>> canonical_path)
>>>>
>>>
>>> I considered to change the name of the argument but decided against
>>> it. The sources of the argument can come from a ClassPathEntry name in some
>>> of the places. It creates confusion without also change the other existing
>>> code. A separate exercise can be used to clean up the existing code.
>>>
>>>
>>>>
>>>> class ClassPathImageEntry ... {
>>>> + // returns canonical path of <JAVA_HOME>/lib/<MODULES_IMAGE_NAME>
>>>> const char* name() const { return _name == NULL ? "" : _name; }
>>>>
>>>>
>>>> [2] 142 const char* ClassLoader::_modules_image_identity =
>>>> MODULES_IMAGE_NAME;
>>>>
>>>> ... but your version of is_modules_image() assumes the uninitialized
>>>> value is NULL.
>>>>
>>>
>>> Changed to initialize it to NULL.
>>>
>>>
>>>>
>>>> [3] For the following code:
>>>>
>>>> 452 static bool is_modules_image(const char* name) {
>>>> 453 if (Arguments::has_jimage()) {
>>>> 454 assert(modules_image_identity() != NULL, "must be set");
>>>> 455 return strcmp(name, modules_image_identity()) == 0;
>>>> 456 }
>>>> 457 return false;
>>>> 458 }
>>>>
>>>> Instead of maintaining a copy of the jimage's name in
>>>> _modules_image_identity, how about
>>>>
>>>> const char* modules_image_identity() {
>>>> assert(get_jrt_entry() != NULL, "must be inited");
>>>> return get_jrt_entry()->name();
>>>> }
>>>>
>>>> Also, I think in most cases, the name is exactly the same pointer as
>>>> get_jrt_entry()->name(), so line 455 can be changed to
>>>>
>>>> const char* id = modules_image_identity();
>>>> return name == id || strcmp(name, id) == 0;
>>>>
>>>> Also, I think you should change ClassPathImageEntry::is_modules_image()
>>>> to simply return true. Otherwise it's just comparing its name with its name
>>>> .... If you do this, then you can add another assert to my version of
>>>> modules_image_identity(), without worrying above infinite recursion:
>>>>
>>>> assert(get_jrt_entry()->is_modules_image(), "must be");
>>>>
>>>
>>> I'm hesitate to use ClassPathImageEntry name directly because it may
>>> not necessary to be full canonical path. Using a separate variable for
>>> modules identity seems to be more explicit and less error-prone in the
>>> future.
>>>
>>> But your code essentially just makes a copy of get_jrt_entry()->name()?
>>> How does that address your concern?
>>>
>>
>> True, it makes an assumption about the existing implementation, which
>> might not be future-proof. One solution is to change
>> create_class_path_entry() to give back the canonical path explicitly.
>>
>> Our problem is not strictly limited to canonical paths. All of the
>> callers that expect a positive return from is_modules_image() get their
>> input from get_jrt_entry()->name(). For example, see
>> ClassLoader::load_class(). The caller wants to know "is the src, supplied
>> in a ClassFileStream, the same as get_jrt_entry()".
>>
>> -> Just grep with 'is_modules_image[(][^)]'. There are very few callers.
>>
>> The problem is get_jrt_entry()->name() may be subject to transformation,
>> so that it's not necessarily JAVA_HOME/lib/modules. In this particular bug,
>> the transformation is pathname canonicalization. It's possible to use other
>> transformations. E.g., change it to "?modules?".
>>
>> So the check we really want to do is: strcmp(name,
>> get_jrt_entry()->name());
>>
>
> There are mixed sources of the path strings when calling into
> is_modules_image(const char*). The ClassFileStream::_source is one and
> ClassPathEntry->name() is another case. The ClassFileStream::_source is the
> canonical path of the class file stream.
>
>
> ClassLoader::load_class() {
> ...
> stream = _jrt_entry->open_stream(file_name, CHECK_NULL);
> ...
>
> ClassFileStream* ClassPathImageEntry::open_stream(const char* name, TRAPS)
> {
> ...
> return new ClassFileStream((u1*)data,
> (int)size,
> _name,
> ClassFileStream::verify);
>
>
> So the ClassFileStream contains the same string as in the _jrt_entry?
>
> Where do you see this "_source is the canonical path of the class file
> stream" happening?
>
You are right, the ClassFileStream created for the runtime image uses
ClassPathImageEntry::_name as the _source. So there is only a single source
of the path string, which is jrt_entry '_name'. Using that within the check
probably is ok then.
Thanks,
Jiangli
>
>
> If the ClassPathEntry->name() changes as you describe above (which is also
> the source of my hesitation), then using get_jrt_entry()->name() in the
> check would cause an issue. That's also the reason why I didn't think a
> simple strcmp was a good idea.
>
> If you are pushing to use get_jrt_entry()->name() and do a simple strcmp,
> then it needs to guarantee get_jrt_entry()->name() will always give the
> full canonical path. Not sure if a comment with a warning
> in ClassPathImageEntry class is sufficient?
>
> Thanks,
> Jiangli
>
>> Thanks
>>
>>
>> Thanks,
>> Jiangli
>>
>>>
>>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> On 3/13/19 10:22 AM, Jiangli Zhou wrote:
>>>>
>>>>
>>>>
>>>> On Tue, Mar 12, 2019 at 4:42 PM Jiangli Zhou <jianglizhou at google.com>
>>>> wrote:
>>>>
>>>>> One of my early experimental revision actually simply used strcmp to
>>>>> compare the full path and to also recorded the full path for the canonical
>>>>> modules name.
>>>>>
>>>>> I decide to not go with that because there is no guarantee that the
>>>>> path given to is_modules_image check is in canonical form and it diverts
>>>>> from the existing assumptions about the usages of this API (checking the
>>>>> file name only). The "/dev/fd/0" example you have is not a direct result of
>>>>> the current fix but is an inherent issue of the modules check. Any file
>>>>> with the same designated modules name would pass the check. If we want to
>>>>> safely use the full path comparison, we need to closely exam all the call
>>>>> sites and make sure all the places use canonical paths. I'll some analysis
>>>>> on that.
>>>>>
>>>>
>>>> I've double checked all existing call sites of is_modules_image(const
>>>> char*). No extra processing is done (e.g. extracting the file name) on the
>>>> path strings when passing to is_modules_image. Using the full string
>>>> comparison appears to be safe so I have no objection. I've reverted my
>>>> change to record the complete canonical path as the modules identity and
>>>> changed to do full path string comparison:
>>>>
>>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.02/
>>>>
>>>> Local testing looks good. I've also submitted a submit-repo testing.
>>>> Please help rerun tier2, tier3 and maybe some of the higher tier tests due
>>>> to the semantics change of is_modules_image.
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>> On Tue, Mar 12, 2019 at 4:13 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>
>>>>>> This seems more complicated that it needs to be.
>>>>>>
>>>>>> + The VM can have at most one modules file.
>>>>>>
>>>>>> + The only reason the VM calls is_modules_image(p) is to check
>>>>>> whether p, a full path, is THE modules file. In all cases, p is actually
>>>>>> derived from the singleton ClassPathImageEntry::name(). It's just that in
>>>>>> some cases, such as in the logging code, we have lost the pointer to the
>>>>>> ClassPathImageEntry, and just have the full path.
>>>>>>
>>>>>> + On line 722 of
>>>>>> http://cr.openjdk.java.net/~jiangli/8220095/webrev.01/src/hotspot/share/classfile/classLoader.cpp.sdiff.html,
>>>>>> you already have a full path of THE modules file.
>>>>>>
>>>>>> Why can't you just do a strcmp of the full path? That's much simpler,
>>>>>> and more importantly, correct. Your version will return true for
>>>>>> "/dev/fd/0" for the test case described in the bug report. That doesn't
>>>>>> seem to be correct.
>>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 3/12/19 9:10 AM, Jiangli Zhou wrote:
>>>>>>
>>>>>> Hi Ioi,
>>>>>>
>>>>>> Thanks for looking into this also.
>>>>>>
>>>>>> On Mon, Mar 11, 2019 at 8:19 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>>>>
>>>>>>
>>>>>> The current bug addresses the practical issue when the modules is a
>>>>>> sym link to a unique content identifier. Using modules as a link to any
>>>>>> arbitrary file is not addressed by this bug. It's in the same area but a
>>>>>> different issue.
>>>>>>
>>>>>> If lib/modules ->lib/test.jar, or lib/modules -> ../ (or any other
>>>>>> arbitrary file/dir) is done, the VM fails differently. This is the same as
>>>>>> if you replace libjvm.so with another random file.
>>>>>>
>>>>>> The canonical modules name is obtained from the actual modules image
>>>>>> during setup_boot_search_path, so it's guaranteed to refer to the correct
>>>>>> identifier. Checking the file name after '/' is safe.
>>>>>>
>>>>>> The substring issue that you pointed out has not been observed, as
>>>>>> that's prevented by the naming method of the content identifiers. The
>>>>>> substring issue however can be solved relatively inexpensively by making
>>>>>> sure the check covers the entire file name after '/'.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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;
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> The argument passed can be a path to the modules, so strcmp cannot be
>>>>>> simply used for 'name'.
>>>>>>
>>>>>> static bool is_modules_image(const char* name) {
>>>>>>
>>>>>> if (Arguments::has_jimage()) {
>>>>>>
>>>>>> assert(_modules_image_name != NULL, "must be set");
>>>>>>
>>>>>> const char* p = skip_directories(name);
>>>>>>
>>>>>> return strcmp(p, _modules_image_name) == 0;
>>>>>>
>>>>>> }
>>>>>>
>>>>>> return false;
>>>>>>
>>>>>> }
>>>>>>
>>>>>>
>>>>>> I've revised the is_modules_image() to also cover the substring issue.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> This function should be changed to always return true
>>>>>>>
>>>>>>> bool ClassPathImageEntry::is_modules_image() const {
>>>>>>> - return ClassLoader::is_modules_image(name());
>>>>>>> + return true;
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> Leaving ClassPathImageEntry::is_modules_image() without changing is
>>>>>> safer, so it can make sure the ClassPathImageEntry name agrees with the
>>>>>> canonical modules name.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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;)
>>>>>>> ...
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> The 'assert(_modules_image_name != NULL, "must be set")' check makes
>>>>>> sure is_modules_image(const char*) is not called
>>>>>> before setup_boot_search_path. There is no need to add additional flag.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>> >>>>
>>>>>>> >>>> -
>>>>>>> >>>>
>>>>>>> >
>>>>>>>
>>>>>>>
>>>>>>
>>>>
More information about the hotspot-runtime-dev
mailing list