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

[David Holmes]

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