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

[Dmitry Samersoff]

Thomas,

I share all David's concerns.

Both Linux and BSD return NULL if size for realloc is zero.

IMHO, if we call realloc with size == 0 it means a program error on
higher level and we should fix it rather than hide.

So it might be better to assert size > 0 inside realloc and return NULL.

-Dmitry


On 2017-02-05 13:56, 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>
> 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
>>>
>>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.