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

[David Holmes]

On 9/06/2014 11:43 PM, Zhengyu Gu wrote:
> 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?

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?

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

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.

Thanks,
David

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