Round 2 RFR (S) 6424123: JVM crashes on failed 'strdup' call.
Dmitry Samersoff
dmitry.samersoff at oracle.com
Wed Jun 11 11:42:17 UTC 2014
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.
More information about the hotspot-runtime-dev
mailing list