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

Zhengyu Gu zhengyu.gu at oracle.com
Mon Jul 28 18:32:14 UTC 2014 

Hi Coleen,
>> I agree with Zhengyu.   I think this is better than doing strdup at 
>> all of the call sites also.  It looks like these NamedCounters do 
>> leak the string here:
>>
>> void FastLockNode::create_rtm_lock_counter(JVMState* state) {
>> #if INCLUDE_RTM_OPT
>>   Compile* C = Compile::current();
>>   if (C->profile_rtm() || (PrintPreciseRTMLockingStatistics && 
>> C->use_rtm())) {
>>     RTMLockingNamedCounter* rlnc = (RTMLockingNamedCounter*)
>>            OptoRuntime::new_named_counter(state, 
>> NamedCounter::RTMLockingCounter);
>>     _rtm_counters = rlnc->counters();
>>     if (UseRTMForStackLocks) {
>>       rlnc = (RTMLockingNamedCounter*)
>>            OptoRuntime::new_named_counter(state, 
>> NamedCounter::RTMLockingCounter);   <= never deallocates duplicated 
>> string in this function
>>       _stack_rtm_counters = rlnc->counters();
>>     }
>>   }
>> #endif
>> }
>>
>> I don't this is for Zhenyu to fix though.  I think Zhenyu has 
>> provided a destructor to use to deallocate the string properly.
>
> Can you file a bug for this if you agree with my assertion that there 
> is a leak here?
>
Yes, I will file a bug.

Thanks,

-Zhengyu


>>
>> MethodOptionMatcher is debatable though.  It does copy the option 
>> each time, but maybe the copy should be outside MethodOptionMatcher 
>> since the reason for copying is that the name is on the stack in the 
>> caller of add_option_string(). This one maybe shouldn't have the 
>> destructor change.
>
> I meant to say that this is a matter of opinion and looks fine if you 
> want to keep it.
>
> Again, I think this change is good.
>
> Coleen
>>
>> fprofiler looks pretty clear cut that copying the string inside the 
>> class is better.
>>
>> Coleen
>>>
>>> CC'd compiler mail list.
>>>
>>> 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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list