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

[Coleen Phillimore]

Hi, I think this is potentially a really good cleanup.  May I suggest 
making DuplicateString an os function like os::duplicate_string() and 
have it always throw OOM, so that the alloc_failmode parameter doesn't 
look inconsistent.

If new code wants to call os::strdup() and check for null return, it can 
do that.  Otherwise call os::safe_duplicate_string() or 
os::duplicate_string() as a wrapper to a os::strdup() that doesn't 
return null.

I'm not convinced the original bug is caused by missing a null check to 
strdup (unless it passed into strdup an already null string). But this 
cleans up using this unsafe function directly.

Thanks,
Coleen

On 6/9/14, 11:50 PM, David Holmes wrote:
> 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
>>>>>>
>>>>
>>