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

[Coleen Phillimore]

On 6/11/14, 4:02 PM, David Holmes wrote:
> 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.

I assume that in general the calls to os::strdup() will check for null.  
Zhengyu's change would add a wrapper in the cases that he's identified 
that do not check for null, and for other cases which might be 
identified later.

He said so earlier but os::malloc and os::<real os call name> imply that 
they act like the os version of these calls.  These calls do not handle 
allocation failure.   It's only a new addition to the "new" and 
"AllocateHeap" calls.  So our JVM named function can do something like 
check for out of memory because it has called os::strdup() which doesn't.

I think it's down to opinion and personal preferences now.  I don't 
think there's too much surprise in this change.

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