RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Ioi Lam
ioi.lam at oracle.com
Wed Mar 13 19:21:45 UTC 2019
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
> <mailto: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
>> <mailto: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());
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 <mailto: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 <mailto: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 <mailto: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
>>>> <mailto: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>
>>>> >>>> <mailto: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