RFR(xs): 8168542: os::realloc should return a valid pointer for input size=0

[Thomas Stüfe]

Hi David,

On Tue, Oct 25, 2016 at 6:22 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 24/10/2016 11:12 PM, Thomas Stüfe wrote:
>
>> Dear all,
>>
>> please check this tiny bug fix.
>>
>> Bug report:
>> https://bugs.openjdk.java.net/browse/JDK-8168542
>>
>> Webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reallo
>> c_size_0/webrev.00/webrev/
>>
>> In short, this fixes a corner case for os::realloc() which currently
>> returns NULL if input size is zero.
>>
>> But as we have coding which interprets a return value of NULL as OOM (See
>> ReallocateHeap()), this is not a good solution. It is also inconsistent
>> with how os::malloc() deals with the same situation and potentially with
>> the way the native C-Runtime deals with it (currently, in a debug build we
>> will return NULL in case of size=0 whereas in the release build we just
>> call the native ::realloc() and return whatever it returns.)
>>
>
> Sorry but I do not like this. A native realloc with a size of zero and a
> non-NULL ptr acts like free(ptr). Our realloc does not do that. A native
> malloc that receives a size of zero "returns either NULL, or a unique
> pointer value that can later be successfully passed to free()". Our
> os::malloc returns 1 - and I see nothing that indicates that can
> successfully be passed to os::free.
>
> So while the current handling of size==0 is a bit inconsistent and
> unclear, it is even less clear that returning 1 is a reasonable thing to do.
>
> To me passing a size of zero (unless expecting it to act like a free!) is
> a bug that should be handled in the caller.
>
>
You completely lost me here. I do not return 1, neither does os::malloc().

os::realloc behaviour now is:
- in debug: if size==0, do not free but return NULL immediately. Which is
not a standard behaviour of a normal ::realloc()
- in release builds: if size==0, do whatever the C-Runtime realloc() does,
so it will always free() but either return NULL or a unique pointer.

So, in debug build the behaviour is unexpected and - assuming os::realloc()
mimicks ::realloc() - wrong. In release builds it will be correct but
unknown.

My patch changes this behaviour to always - in both release and debug
builds - free() and return a unique pointer. By setting the size to 1:

- for the debug build, I will go thru the normal path - allocating 1 byte
of memory (plus NMT/guard pages overhead), copying 1 byte of payload, then
freeing the original memory and returning the alloced 1 byte - which is
unique and can be passed to os::free().
- for the release build I will force the behaviour to be a realloc to size
1 and thus remove the ambiguity introduced by the native realloc.


> I welcome opinions from others on this.
>
> David
>
> PS. I will be traveling soon and unable to respond to emails until
> Wednesday afternoon at the earliest.
>
> Thank you,
>>
>> Thomas
>>
>>
Kind Regards, Thomas