RFR: 8220095: Assertion failure when symlink (with different name) is used for lib/modules file
Jiangli Zhou
jianglizhou at google.com
Tue Mar 12 16:10:15 UTC 2019
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