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

[Calvin Cheung]

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 
> <mailto: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.
>
>>
>>>
>>> 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.

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;
>>>   }
>>> 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
>>>>>>
>>>