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

[Christian Thalinger]

On Jun 9, 2014, at 6:43 AM, Zhengyu Gu <zhengyu.gu at oracle.com> 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?
> 
>>>> 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()

That function name does not match our guidelines:

https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Names

I’m aware that other functions in this file (AllocateHeap, FreeHeap, …) use this pattern but that doesn’t mean we have to introduce new ones.

> , should be the same way to determine whether uses os::malloc() or AllocateHeap(), in my opinion.
> 
> 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
>>>>> 
>>> 
>