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