Round 2 RFR (S) 6424123: JVM crashes on failed 'strdup' call.

[Coleen Phillimore]

On 6/11/14, 8:26 AM, David Holmes wrote:
> On 11/06/2014 9:42 PM, Dmitry Samersoff wrote:
>> Zhengyu,
>>
>> I'm second to David.
>>
>> The number of situations where we should do anything besides standard
>> OOM procedure on failed strdup is limited.
>>
>> So I would suggest to:
>>
>> a) change all calls of ::strdup() to os::strdup()
>> b) create os::strdup_without_result_check()
>> c) change os::strdup() to handle OOM internally
>> d) remove unnecessary checks from os::strdup() call sites.
>
> That's not quite what I'd advocate. os::strdup can have a default arg 
> that specifies the failure mode, so no need for two methods. The 
> default should be to return null. Callers that don't check for null 
> should request the "exit on OOM".

This isn't what I advocated either.

a.  Change all calls of ::strdup() to os::strdup()  yes
b.  Keep os::strdup() to not check and return null
c.  Add os::duplicate_string() to check for null and call 
vm_exit_out_of_memory() as wrapper for os::strdup().  The calls that 
Zhengyu created for StringDuplicate that weren't checking for NULL would 
become os::duplicate_string().
d.  Keep checks from os::strdup() calls.

I didn't like os::strdup() to have a default argument for failure mode 
because it wasn't consistent with other os:: calls.

Coleen
>
> David
> -----
>
>> -Dmitry
>>
>> On 2014-06-11 06:08, David Holmes wrote:
>>> On 10/06/2014 11:37 PM, Zhengyu Gu wrote:
>>>> Hi David,
>>>>
>>>> On 6/9/2014 11:50 PM, David Holmes wrote:
>>>>> I am not sure what needs to be checked?
>>>>>
>>>>> The result of calling os::strdup - else we haven't fixed the original
>>>>> bug. Unless you are saying that all uses of os::strdup do check the
>>>>> return value somewhere in the call chain?
>>>>>
>>>> Thanks for the clarification. That is what the main purpose of this
>>>> change. As I mentioned in earlier email, I intended to replace the 
>>>> call
>>>> sites that
>>>> do not check return values with the new function that handles null
>>>> return values.
>>>>
>>>>
>>>>>>
>>>>>> To determine whether uses os::strdup() or DuplicationString(),
>>>>>> should be
>>>>>> the same way to determine whether uses os::malloc() or 
>>>>>> AllocateHeap(),
>>>>>> in my opinion.
>>>>>
>>>>> AllocateHeap should only be being used as the implementation for
>>>>> CHeapObj and related types, it is not a general purpose allocation
>>>>> interface. So I don't see that the analogy holds. I'd rather see only
>>>>> os::strdup used with all the clients doing the null check (somewhere
>>>>> in the stack) and handling it appropriately.
>>>>>
>>>> That is not true. AllocateHeap() is widely used, all NEW_C_HEAP_xxx
>>>> macros are wrappers of AllocateHeap(). According to comments, the 
>>>> macros
>>>> are primary functions for allocating memory.
>>>
>>> Exactly! The macros are the API to allocation and AllocateHeap is an
>>> internal implementation detail. There is really no analogy with
>>> os::strdup and and new DuplicateString function.
>>>
>>> IMHO the right change here is that every strdup call becomes os::strdup
>>> (for tracking purposes) and that every call to os::strdup is checked 
>>> for
>>> a null return. And if you like you can implement that by having those
>>> sites that don't check call os::strdup(str, EXIT_ON_OOM) - though it
>>> would be better to have a better error response where possible (but 
>>> that
>>> can be deferred to a second RFE).
>>>
>>> Thanks,
>>> David
>>>
>>>> // The following macros and function should be used to allocate memory
>>>> // directly in the resource area or in the C-heap, The _OBJ variants
>>>> // of the NEW/FREE_C_HEAP macros are used for alloc/dealloc simple
>>>> // objects which are not inherited from CHeapObj, note constructor and
>>>> // destructor are not called. The preferable way to allocate objects
>>>> // is using the new operator.
>>>> //
>>>> // WARNING: The array variant must only be used for a homogenous array
>>>> // where all objects are of the exact type specified. If subtypes are
>>>> // stored in the array then must pay attention to calling destructors
>>>> // at needed.
>>>> //
>>>> //   NEW_RESOURCE_ARRAY(type, size)
>>>> //   NEW_RESOURCE_OBJ(type)
>>>> //   NEW_C_HEAP_ARRAY(type, size)
>>>> //   NEW_C_HEAP_OBJ(type, memflags)
>>>> //   FREE_C_HEAP_ARRAY(type, old, memflags)
>>>> //   FREE_C_HEAP_OBJ(objname, type, memflags)
>>>> //   char* AllocateHeap(size_t size, const char* name);
>>>> //   void  FreeHeap(void* p);
>>>> //
>>>>
>>>> A quick grep shows that hotspot repo has about 40 instances of
>>>> os::malloc(), but close to 400 instances NEW_C_HEAP_ARRAY* alone.
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>
>>>>
>>
>>