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

Wed Jul 23 17:33:35 UTC 2014

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