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

[Zhengyu Gu]

Try again,

> ---
>
> src/share/vm/compiler/compilerOracle.cpp
>
> It's not obvious that your change is correct. If MethodMatcher::match 
> is called several times might you not want to duplicate the string on 
> each call? That said it isn't obvious that these strings need copying 
> at all - we'd need to trace their complete lifecycle.
>
> Similar comments for NamedCounter in runtime.cpp and vmNode in 
> fprofiler.cpp
>
> Having to add all those destructors seems messy and it isn't evident 
> that it is necessary.
>

Can someone from compiler team take a look?

Webrev:http://cr.openjdk.java.net/~zgu/6424123/webrev.02 
<http://cr.openjdk.java.net/%7Ezgu/6424123/webrev.02>

Thanks,

-Zhengyu


> ---
>
> Thanks,
> David
>
> On 11/07/2014 3:39 AM, 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
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140723/2fc5b36b/attachment.html>