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

[Dmitry Samersoff]

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.

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.