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

[Zhengyu Gu]

I am more aligned with Coleen's proposal. I would like keep 
os::malloc()/os::free()/os::realloc()/os::strdup() as replacement of c 
library functions, and keep their semantics close enough. I will replace 
DuplocateString() with os::duplicate_string() with internal OOM handling.

Let me know if it is OK with everyone.

Thanks,

-Zhengyu


On 6/11/2014 9:39 AM, 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
>>>>>
>>>>>
>>>>>
>>>
>>>
>