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

[Coleen Phillimore]

Zhengyu,
This looks good.  One minor comment, can you either add a comment that 
this doesn't call the c runtime library strdup or call as os::strdup?

  518   char* p = strdup(str, flags);


It's obvious if you know that there's a strdup in os:: or know c++ well 
enough to expect ::strdup to call the c runtime library one, but I think 
a tiny note would be helpful.

Thanks,
Coleen

On 7/10/14, 1:39 PM, Zhengyu Gu wrote:
> Sorry for the long delay. The update is mainly based on Coleen's 
> suggestion.
>
> DuplicateString()  is changed to os::strdup_check_oom().
>
> Bug:https://bugs.openjdk.java.net/browse/JDK-6424123 
> <https://bugs.openjdk.java.net/browse/JDK-6424123>
> Webrev:http://cr.openjdk.java.net/~zgu/6424123/webrev.02/ 
> <http://cr.openjdk.java.net/%7Ezgu/6424123/webrev.02/>
>
>
> Thanks,
>
> -Zhengyu
>
> <http://cr.openjdk.java.net/%7Ezgu/6424123/webrev.02/>
>
>
> On 6/10/2014 5:30 PM, Coleen Phillimore wrote:
>>
>> 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
>>>>>>>>
>>>>>>
>>>>
>>
>