RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Wed Mar 13 20:45:08 UTC 2019
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. 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