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

Mon Feb 6 08:13:42 UTC 2017

Hi David,

On Sun, Feb 5, 2017 at 11:43 PM, David Holmes <david.holmes at oracle.com>
wrote:

> 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.
>
>
And on release builds you would return NULL?

I grant asserting would catch some of the accidental size=0 cases, but you
cannot be sure to hit that code path in the debug build.

> 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 think the biggest problem is that malloc/realloc returning NULL is
usually thought of as OOM. Nobody checks the input size. That will confuse
error analysis.

See also the comment in os::malloc(), which states the same as a reason for
the 1-byte allocation.

As for 1-byte allocations worrying you, we could do the pointer-to-sentinel
proposal I explained in my last mail if we are worried about memory leaks.
os::malloc()/os::realloc() and os::free() have to be perfectly matched
today anyway because of the malloc headers and NMT.

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

Sure, this is fine for me. Thank you for your review!

Kind Regards, Thomas

> 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_reallo
>> c_size_0/jdk10-webrev.01/webrev/
>>         <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reall
>> oc_size_0/jdk10-webrev.01/webrev/>
>>
>>         For reference, the old discussion back in october 16:
>>         http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>> 016-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
>>
>>
>>