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

[David Holmes]

On 11/06/2014 11:39 PM, Coleen Phillimore wrote:
>
> 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.

Given allocation failure handling is a relatively new addition that 
isn't too surprising. But the most consistent approach, considering 
os::malloc, would be to always check the return value. Anywhere we would 
be adding this new duplicate_string call why not just add a regular 
os::strdup and check the return value and decide how to respond? When we 
look at the actual call sites "exit on OOM" may not be the right error 
strategy.

Cheers,
David

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