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

[David Holmes]

On 8/06/2014 10:48 PM, Zhengyu Gu wrote:
> Hi David,
>
> On 6/8/2014 5:03 AM, David Holmes wrote:
>> Hi Zhengyu,
>>
>> Still a bit perplexed by the aim here. Why replace non-null checked
>> strdup calls with non-null os::strdup?
> ::strdup() is not tracked by NMT, but os::strdup() is.

Okay, but it still needs to be checked.

>> If the issue is that the result of strdup must be checked then it must
>> be checked. Why add DuplicateString instead of changing what
>> os::strdup does?
> I replaced strdup()/os::strdup() with DuplicateString(), where caller
> does not check null pointer. If caller checks null pointer, I left it
> alone.

That is not apparent from the webrev eg:

src/os/windows/vm/perfMemory_windows.cpp


> DuplicateString() mirrors what AllocateHeap() does, and os::strdup()
> mirrors os::malloc(). AllocateHeap()/DuplicateString() can handle OOM
> (by default), but os::strdup()/os::malloc() do not.

I don't see the need for this duality here. Why do we need to dup 
strings in different memory areas? Why can't os::strdup do the null 
check internally and abort if requested?

Putting it another way if someone writes a new piece of code where they 
need to dup an incoming string, what determines whether they should use 
os::strdup or DuplicateString?

Thanks,
David

> Thanks,
>
> -Zhengyu
>
>>
>>
>> Thanks,
>> David
>>
>> On 7/06/2014 3:05 AM, Zhengyu Gu wrote:
>>> Updated webrev introduces a new DuplicateString() function, that handles
>>> OOM, similar to AllocateHeap(), and replaces the call sites that do not
>>> check NULL pointer with this DuplicateString().
>>>
>>> http://cr.openjdk.java.net/~zgu/6424123/webrev.01/
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>
>>>
>>> On 6/4/2014 4:32 PM, Zhengyu Gu wrote:
>>>> JVM should avoid C library's strdup() and use os::strdup() instead.
>>>> os::strdup() handles OOM, so can avoid JVM crash.
>>>>
>>>> Also, made limited scope of code cleanup, which makes memory ownership
>>>> more explicit, so they can be claimed by object's destructors.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6424123
>>>> Webrev: http://cr.openjdk.java.net/~zgu/6424123/webrev.00/
>>>>
>>>> Tests:
>>>>   - JPRT
>>>>   - vm.quick.testlist on Linux x64.
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>
>