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

[Thomas Stüfe]

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>
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
>>
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reallo
>> c_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
>>
>
> 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
 a os::malloc
size==0: return valid pointer to 1 byte array
 b os::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
 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

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