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

[Jiangli Zhou]

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.

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

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