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

[Jiangli Zhou]

> On Jul 10, 2018, at 5:43 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> 
> 
>> On 7/10/18, 4:09 PM, Jiangli Zhou wrote:
>> Hi Calvin,
>> 
>> 
>> On Jul 10, 2018, at 2:17 PM, Calvin Cheung <calvin.cheung at oracle.com> 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.
>> 
>> That looks good. On the other hand David’s comments also sound reasonable to me. So it’s your call.
> Let's go with webrev.01.

Ok.

>> 
>>> 
>>>> 
>>>> 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 for digging further down. So PathString::set_value() would never return false in this case. It seems we should add a comment in set_value() to avoid future confusion. Also the ‘else’ case can be removed. That can be handled separately.
> The function is being called at eight different places and none of them checks the return value.
> Yes, let's clean it up in a separate bug/RFE.

Please file a new RFE.

Thanks,
Jiangli 

> 
> thanks,
> Calvin
>> 
>> Thanks,
>> Jiangli
>> 
>>> 
>>> 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;
>>>>   }
>>>>   return true;
>>>> }
>>>> 
>>>> Thanks,
>>>> Jiangli
>>>> 
>>>>> On Jul 10, 2018, at 10:31 AM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
>>>>> 
>>>>> Updated webrev with the changes mentioned below:
>>>>>    http://cr.openjdk.java.net/~ccheung/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/
>>>>>>>> 
>>>>>>>> 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
>>>>>>> 
>>>>