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

[David Holmes]

On 11/07/2018 8:27 AM, Calvin Cheung wrote:
> Hi David,
> 
> Thanks for chiming in.
> 
> I take that I should go with webrev.01 of the change?

That's up to you and your reviewers. :)

David

> thanks,
> Calvin
> 
> On 7/10/18, 3:01 PM, David Holmes wrote:
>> 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
>>>>>>>
>>>>