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:53:26 UTC 2019
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?
>
> 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