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

Wed Feb 8 09:01:56 UTC 2017

Hi,

On 02/07/2017 02:24 AM, Chris Plummer wrote:
> 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.

Since there is a while since jdk10 will be GA, now is the best time.

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

I agree that realloc and malloc should do the same.

IEEE Std 1003.1, 2004 Edition says:
"If size is 0 and ptr is not a null pointer, the object pointed to is freed."

So as I said in previously mail, there is nothing wrong with doing realloc(ptr, 0), but I also don't think we should do it hotspot.
Doing malloc(0) is undefined, so this really should be at least an assert.

I'm all favor assert for both realloc and malloc.

I can do pre-testing, KS, jtreg, tonga or whatever we think is appropriate.
(if this is a rat's nest we can change our minds...)

/Robbin

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