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

[Zhengyu Gu]

Thanks a lot!

-Zhengyu

On 7/23/2014 1:33 PM, Vladimir Kozlov wrote:
> In compilerOracle.cpp the collected information is never deleted so 
> destructor will not be called for MethodOptionMatcher. But I am fine 
> to have the destructor (at least for silencing parfait if it complains).
>
> opto/runtime.* changes are fine.
>
> Thanks,
> Vladimir
>
> On 7/23/14 10:00 AM, Zhengyu Gu wrote:
>> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>