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

Tue Feb 7 01:24:44 UTC 2017

I don't think we can push a change to assert when size == 0 without 
first thoroughly testing it and fixing any places that currently do 
that. Testing may turn up nothing, or it may turn up a rat's nest. Just 
something to think about before starting down this path.

I'm not as offended by the size 1 approach as others, and it is what we 
already do for malloc(). It probably  makes sense to have malloc and 
realloc be consistent in this regard, regardless of what the native 
version do by default (which can vary). Just given the fact that we are 
even having this discussion and debating which is best tends to make me 
think that caller's of malloc() and realloc() either didn't even give 
this topic any thought, or possibly came to differing conclusions on 
whether or not it mattered if they passed in a size == 0.

Chris

On 2/6/17 3:49 AM, Dmitry Samersoff wrote:
> 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
>>>>
>>>>
>