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 20:54:22 UTC 2019
On 3/13/2019 1:45 PM, Jiangli Zhou wrote:
> On Wed, Mar 13, 2019 at 12:21 PM Ioi Lam <ioi.lam at oracle.com
> <mailto: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
>> <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());
>
>
> 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.
ClassLoader::load_class() {
...
stream = _jrt_entry->open_stream(file_name, CHECK_NULL);
...
ClassFileStream* ClassPathImageEntry::open_stream(const char* name, TRAPS) {
...
return new ClassFileStream((u1*)data,
(int)size,
_name,
ClassFileStream::verify);
So the ClassFileStream contains the same string as in the _jrt_entry?
Where do you see this "_source is the canonical path of the class file
stream" happening?
> 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
>>>> <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