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

[Coleen Phillimore]

On 6/11/14, 10:27 AM, Dmitry Samersoff wrote:
> Coleen,
>
> I'm OK with your proposal.
>
> Should os::duplicate_string() have clear name like
> os:strdup_with_error_check() ?

I don't have a strong opinion.  Zhengyu can pick.

Coleen

>
> -Dmitry
>
> On 2014-06-11 17:39, 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.
>>
>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>
>