RFR(S): 8205946: JVM crash after call to ClassLoader::setup_bootstrap_search_path()

David Holmes david.holmes at oracle.com
Tue Jul 10 22:01:23 UTC 2018


Calling vm_exit_during_initialization is very unfriendly for 
applications that host the JVM in-process directly. With that in mind 
decisions to call vm_exit_* should done at as a high a level as feasible 
in the code - ie. a low-level method like os::set_boot_path should never 
IMHO be making a decision as to whether an error it encounters is fatal 
to the whole VM initialization process. That's a decision to be made 
higher up.

My 2c.

David

On 11/07/2018 7:17 AM, Calvin Cheung wrote:
> Hi Jiangli,
> 
> Thanks for reviewing.
> 
> On 7/10/18, 12:18 PM, Jiangli Zhou wrote:
>> Hi Calvin,
>>
>> The error handling code in platform specific code are identical. I 
>> like Lois’ suggestion to check and exit in os::set_boot_path() to 
>> avoid duplicating the code.
> If you want changes in os::set_boot_path() instead of in platform 
> specific code, I'm proposing the following:
> diff --git a/src/hotspot/share/runtime/os.cpp 
> b/src/hotspot/share/runtime/os.cpp
> --- a/src/hotspot/share/runtime/os.cpp
> +++ b/src/hotspot/share/runtime/os.cpp
> @@ -1270,7 +1270,7 @@
>     return file;
>   }
> 
> -bool os::set_boot_path(char fileSep, char pathSep) {
> +void os::set_boot_path(char fileSep, char pathSep) {
>     const char* home = Arguments::get_java_home();
>     int home_len = (int)strlen(home);
> 
> @@ -1278,26 +1278,30 @@
> 
>     // modular image if "modules" jimage exists
>     char* jimage = format_boot_path("%/lib/" MODULES_IMAGE_NAME, home, 
> home_len, fileSep, pathSep);
> -  if (jimage == NULL) return false;
> +  if (jimage == NULL) {
> +    vm_exit_during_initialization("Failed setting boot class path.", 
> NULL);
> +  }
>     bool has_jimage = (os::stat(jimage, &st) == 0);
>     if (has_jimage) {
>       Arguments::set_sysclasspath(jimage, true);
>       FREE_C_HEAP_ARRAY(char, jimage);
> -    return true;
> +    return;
>     }
>     FREE_C_HEAP_ARRAY(char, jimage);
> 
>     // check if developer build with exploded modules
>     char* base_classes = format_boot_path("%/modules/" JAVA_BASE_NAME, 
> home, home_len, fileSep, pathSep);
> -  if (base_classes == NULL) return false;
> +  if (base_classes == NULL) {
> +    vm_exit_during_initialization("Failed setting boot class path.", 
> NULL);
> +  }
>     if (os::stat(base_classes, &st) == 0) {
>       Arguments::set_sysclasspath(base_classes, false);
>       FREE_C_HEAP_ARRAY(char, base_classes);
> -    return true;
> +    return;
>     }
>     FREE_C_HEAP_ARRAY(char, base_classes);
> -
> -  return false;
> +  vm_exit_during_initialization("Failed setting boot class path.", NULL);
> +  return;
>   }
> 
> The function fails if os::stat() fails or allocation of buffer for 
> "jimage" or "base_classes" fails.
> Since vm should exit on failure in the above function, it is unnecessary 
> for the function to return a bool.
> 
>>
>> Also, under low memory condition, set_value() might fail to allocate 
>> and not trigger any error with a release binary.  os::set_boot_path() 
>> probably should also check and make sure sys path is not NULL after 
>> Arguments::set_sysclasspath().
> The AllocateHeap() you listed below will call into the following which 
> will exit on out of memory. So I don' think we need to do NULL check again.
> 
> // allocate using malloc; will fail if no memory available
> char* AllocateHeap(size_t size,
>                     MEMFLAGS flags,
>                     const NativeCallStack& stack,
>                     AllocFailType alloc_failmode /* = 
> AllocFailStrategy::EXIT_OOM*/) {
>    char* p = (char*) os::malloc(size, flags, stack);
>    if (p == NULL && alloc_failmode == AllocFailStrategy::EXIT_OOM) {
>      vm_exit_out_of_memory(size, OOM_MALLOC_ERROR, "AllocateHeap");
>    }
>    return p;
> }
> 
> thanks,
> Calvin
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>>     FreeHeap(_value);
>>   }
>>   _value = AllocateHeap(strlen(value)+1, mtArguments);
>>   assert(_value != NULL, "Unable to allocate space for new path value");
>> if (_value != NULL) {
>>     strcpy(_value, value);
>>   } else {
>> // not able to allocate
>> return false;
>>   }
>> returntrue;
>> }
>>
>> Thanks,
>> Jiangli
>>
>>> On Jul 10, 2018, at 10:31 AM, Calvin Cheung <calvin.cheung at oracle.com 
>>> <mailto:calvin.cheung at oracle.com>> wrote:
>>>
>>> Updated webrev with the changes mentioned below:
>>> http://cr.openjdk.java.net/~ccheung/8205946/webrev.01/ 
>>> <http://cr.openjdk.java.net/%7Eccheung/8205946/webrev.01/>
>>>
>>> I've rerun hs-tier{1,2,3} tests.
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 7/9/18, 1:43 PM, Calvin Cheung wrote:
>>>> Hi Lois,
>>>>
>>>> Thanks for your review.
>>>>
>>>> On 7/9/18, 11:58 AM, Lois Foltan wrote:
>>>>> On 7/9/2018 1:29 PM, Calvin Cheung wrote:
>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8205946
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8205946/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Eccheung/8205946/webrev.00/>
>>>>>>
>>>>>> The JVM crash could be simulated by renaming/removing the modules 
>>>>>> file under the jdk/lib directory.
>>>>>> The proposed simple fix is to perform a 
>>>>>> vm_exit_during_initialization().
>>>>>
>>>>> Hi Calvin,
>>>>>
>>>>> Some clarifying questions.  Is this just an issue for exploded builds?
>>>> I don't think so. As mentioned above, I could reproduce the crash 
>>>> with a regular jdk image build by renaming the modules file under 
>>>> the jdk/lib directory.
>>>>>  I would prefer the exit to occur if the os::stat() fails for the 
>>>>> system class path in os::set_boot_path().
>>>> Instead of exiting in os::set_boot_path(), how about checking the 
>>>> return status of os::set_boot_path() in the caller and exiting there 
>>>> like the following:
>>>> bash-4.2$ hg diff os_linux.cpp
>>>> diff --git a/src/hotspot/os/linux/os_linux.cpp 
>>>> b/src/hotspot/os/linux/os_linux.cpp
>>>> --- a/src/hotspot/os/linux/os_linux.cpp
>>>> +++ b/src/hotspot/os/linux/os_linux.cpp
>>>> @@ -367,7 +367,9 @@
>>>>       }
>>>>     }
>>>>     Arguments::set_java_home(buf);
>>>> -    set_boot_path('/', ':');
>>>> +    if (!set_boot_path('/', ':')) {
>>>> +      vm_exit_during_initialization("Failed setting boot class 
>>>> path.", NULL);
>>>> +    }
>>>>   }
>>>>
>>>> Note that before the above change, the return status of 
>>>> set_boot_path() isn't checked.
>>>> The above would involve changing 5 of those os_*.cpp files, one for 
>>>> each O/S.
>>>>
>>>>>  With certainly an added assert later in 
>>>>> ClassLoader::setup_bootstrap_search_path() to ensure that the 
>>>>> system class path is never NULL.
>>>> Sure, I can add an assert there.
>>>> I'll post updated webrev once I've made the change and done testing.
>>>>
>>>> thanks,
>>>> Calvin
>>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>>
>>>>>> Ran hs-tier{1,2,3} tests successfully including the new test case.
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>
>>


More information about the hotspot-runtime-dev mailing list