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 18:03:26 UTC 2019
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?
is_modules_image(const char* name) --> is_modules_image(const char*
canonical_path)
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.
[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");
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