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

[Zhengyu Gu]

Hi David,

Thanks for the review. Please see comments inline.

On 7/11/2014 2:33 AM, David Holmes wrote:
> Hi Zhengyu,
>
> A number of comments ...
>
> First an aside  ... based on the recent header file changes for 
> .iniline.hpp file I would think these pairs:
>
> +#include "memory/allocation.hpp"
> +#include "memory/allocation.inline.hpp"
>
> would reduce to
>
> +#include "memory/allocation.inline.hpp"
>
> if allocation.inline.hpp included allocation.hpp as it apparently should.
>
I will fix them.

> ---
>
> In the vm_version_* changes are you sure os::malloc will be ready to 
> work very early in the initialization sequence of the VM? os::malloc 
> calls out to a lot of other code especially in debug mode - eg: will 
> check_heap() work, GuardedMemory, MemTracker itself?
>
> Similarly with the use of os::strdup_check_oom - will it actually 
> manage to terminate cleanly if OOME happens?
>
> Same comments for arguments.cpp changes. It may be too early to call 
> os::strdup_check_oom.
>
The earliest os::malloc() calls are from initialization of static 
objects, ahead of vm_version_* initialization, so I don't think it is an 
issue.

> ---
>
> 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.
I moved 'strdup' from caller to the class,  I think it makes the 
ownership more explicit. I can not tell if the string needs to be 
duplicated, or vmNode, NamedCounter ever to be released. If so, it looks 
to me like memory leaks.

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