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

[David Holmes]

Hi Thomas,

> But if we follow your suggestion and make size==0 an assertable
 > offense, we should do so too for malloc(0).

Well that would be my preference if doing this API today. There is 
supposed to be a difference between ASSERT and !ASSERT code - the former 
is supposed to help trap programming errors. So asserting on an 
allocation of size zero seems reasonable to me. Of course we may find 
places in our code that then hit the assert.

I do agree that your proposal makes things consistent - I just don't 
like the allocate-1-byte approach (malloc/realloc won't be able to 
allocate a single byte anyway so this will be some minimum size).

I'm going to leave this to others to chime in. I can accept your patch 
so if others are okay with it then that's fine.

Thanks,
David
-----

On 5/02/2017 8:56 PM, Thomas Stüfe wrote:
> Hi David,
>
> some context: the whole issue came into being because for a long time we
> had exchanged os::malloc/os::realloc with our own versions for malloc
> tracking purposes. This was way before NMT.
>
> When writing the os::malloc/realloc replacements and associated tests,
> we took care to mimick the behavior of native malloc/realloc in a
> consistent matter. That way we could exchange malloc calls with my
> os::malloc on a case by case base without behavior change (or put it
> differently, nothing would break if I accidentally miss an
> instrumentation in code and leave it a native malloc/realloc)
>
> Now we went back to the OpenJDK version of os::malloc()/os::realloc() to
> reduce maintenance, and my cornercase tests fail. Hence this bug report.
>
> Please find further answers inline.
>
> On Sun, Feb 5, 2017 at 3:50 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     On 4/02/2017 10:09 PM, Thomas Stüfe wrote:
>
>         Hi guys,
>
>         picking up this trivial issue which got pushed to jdk10, could I
>         please
>         have a re-review? Oh, also a sponsor.
>
>         Issue:
>         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/jdk10-webrev.01/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.01/webrev/>
>
>         For reference, the old discussion back in october 16:
>         http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-October/021675.html
>         <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-October/021675.html>
>
>
>     I am still rather confused by both existing code and proposed change
>     - the code just seems wrong if passed in a zero size. Passing in
>     zero is a bug AFAICS and if buggy code passes in zero then how do we
>     know what it will do with the returned value from os::realloc ?
>
>
>
> Currently, in the OpenJDK version, we have the following behavior:
>
> 1 ASSERT
>  aos::malloc
> size==0: return valid pointer to 1 byte array
>  bos::realloc
> ptr != NULL && size==0: return NULL
>
> 2 !ASSERT
>  a      os::malloc
> size==0: return valid pointer to 1 byte array
>  b      os::realloc
> ptr != NULL && size==0: whatever the native realloc does, which may be
> returning NULL or returning a valid allocation.
>
> This is inconsistent.
>
> A) 1a and 1b, although arguably similar cases, behave differently.
> B) 1b and 2b may behave differently, depending on the behaviour of the
> native CRT calls. Which is IMHO worse than case (A). You do not want
> different behaviour between ASSERT and !ASSERT.
>
> Additionally, for (B) it is not good that we delegate cornercase
> behavior to the native CRT, because that may lead to os::realloc() to
> have different behavior between platforms. On one platform,
> ::realloc(ptr,0) may return NULL, on another it may return a pointer. So
> you get platform dependent errors.
>
> My patch proposes to change the behavior like this:
>
> 1 ASSERT
>  aos::malloc
> size==0: return valid pointer to 1 byte array
>  bos::realloc
> ptr != NULL && size==0: return valid pointer to 1 byte array
>
> 2 !ASSERT
>  a      os::malloc
> size==0: return valid pointer to 1 byte array
>  b      os::realloc
> ptr != NULL && size==0: return valid pointer to 1 byte array
>
> This is more consistent.
>
> -----------
>
> Aside from the consistency issue:
>
> I think if someone passes 0 as resize size, this may mean he calculated
> the resize size wrong. Note that this does not necessarily mean a fatal
> error, if he has the correct notion of buffer size - 0 - and does not
> overwrite anything, nothing bad will happen.
>
> In this situation you have three choices
>
> 1) return NULL. Caller will either incorrectly access the pointer and
> crash, or test the pointer and all cases I saw then assume OOM. Also bad.
>
> 2) assert. This just seems plain wrong. Inconsistent with CRT behavior,
> and what do you do in release code?
>
> 3) return a 1 byte allocation. That is what I propose. This may in the
> worst case lead to memory leaks; but only if caller expected the
> behaviour of os::realloc(ptr, 0) to be "free ptr and return NULL", which
> I have seen nowhere. If caller is oblivious to giving us size 0, he will
> be freeing the memory later.
>
> (Note that you could easily prevent memory leaks by just returning a
> special global static sentinel pointer for size==0, but you have to be
> sure that all os::realloc calls are matched by os::free calls.)
>
> I see your point about treating size==0 as an error. But I am not a big
> fan, for the mentioned reasons, and would prefer os::realloc() to behave
> like native realloc.
>
> But if we follow your suggestion and make size==0 an assertable offense,
> we should do so too for malloc(0).
>
> Kind Regards, Thomas
>
>
>
>
>     David
>
>
>         The webrev is unchanged from the proposal for jdk9, just rebased to
>         jdk10/hs.
>
>         @Chris, I decided to not follow your suggestion to move the
>         comparison
>         into the #ifndef assert. You are right, this is redundant with
>         the check
>         inside os::malloc(), but as you said, checking at the entry of
>         os::realloc() makes it clearer.
>
>         Thank you!
>
>         Kind Regards, Thomas
>
>