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

[Zhengyu Gu]

Hi David,

On 6/8/2014 5:59 PM, David Holmes wrote:
> 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.
>
I am not sure what 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
>
There are several places that are not obvious. Sometimes, I have to go a 
few levels to find out how the pointers are used.

For this particular case, sharedmem_fileName is only referenced by 
PerfMemory::backing_store_filename(), but there is no caller to this 
function.

$ grep -r backing_store_filename *
os/aix/vm/perfMemory_aix.cpp:char* PerfMemory::backing_store_filename() {
os/bsd/vm/perfMemory_bsd.cpp:char* PerfMemory::backing_store_filename() {
os/linux/vm/perfMemory_linux.cpp:char* 
PerfMemory::backing_store_filename() {
os/solaris/vm/perfMemory_solaris.cpp:char* 
PerfMemory::backing_store_filename() {
os/windows/vm/perfMemory_windows.cpp:char* 
PerfMemory::backing_store_filename() {
share/vm/runtime/perfMemory.hpp:    static char* backing_store_filename();

I thought parfait should be good at finding these things.

>
>> 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?
>
I agree it is confusing that we have some many memory allocation 
functions to accomplish similar things, given AllocateHeap() and 
ReallocateHeap() can also return NULL now. That's also the reason to get 
me wrong in the first iteration.

The purposed approach at least keep os::malloc(), os::realloc() and 
os::strdup() as replacement of c library's counterparts.
Having os::strdup() handles OOM, but not os::malloc() and os::realloc(), 
seems to me even more confusing and inconsistent.

To determine whether uses os::strdup() or DuplicationString(), should be 
the same way to determine whether uses os::malloc() or AllocateHeap(), 
in my opinion.

Thanks,

-Zhengyu


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