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

[Zhengyu Gu]

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