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

[Jiangli Zhou]

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.

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

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