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

[Calvin Cheung]

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