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