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

[David Holmes]

On 25/10/2016 3:50 PM, Thomas Stüfe wrote:
> Hi David,
>
> On Tue, Oct 25, 2016 at 6:22 AM, David Holmes <david.holmes at oracle.com
> <mailto: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
>         <https://bugs.openjdk.java.net/browse/JDK-8168542>
>
>         Webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/webrev.00/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_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().

Sorry some kind of visual-neural short-circuit. :)

Okay size 0 becomes size 1.

Let me just recant my email and let someone else step in. Thanks for the 
detailed explanation below.

David

> 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