Quantcast

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

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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_realloc_size_0/jdk10-webrev.01/webrev/

For reference, the old discussion back in october 16:
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-October/021675.html

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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
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_realloc_size_0/jdk10-webrev.01/webrev/
>
> For reference, the old discussion back in october 16:
> 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 ?

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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
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 <[hidden email]>
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
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

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

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 <[hidden email]
> <mailto:[hidden email]>> 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_realloc_size_0/jdk10-webrev.01/webrev/
>         <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.01/webrev/>
>
>         For reference, the old discussion back in october 16:
>         http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-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
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi David,

On Sun, Feb 5, 2017 at 11:43 PM, David Holmes <[hidden email]>
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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
In reply to this post by Thomas Stüfe-2
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 <[hidden email]>
> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Robbin Ehn
In reply to this post by Thomas Stüfe-2
Hi Thomas,

realloc with size 0 is equivalent to free, not an error generally speaking.
But in jvm context I think we should not do realloc with size 0.

So instead of getting an OOM it's much better to crash so we can find out who tried realloc 0.

  ALWAYSINLINE char* ReallocateHeap(char *old, size_t size, MEMFLAGS flag,
      AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM) {
+  guarantee(size != 0, "Reallocation size must be non-zero");

Or if we (unlikely) think realloc size 0, is okay:

-  if (p == NULL && alloc_failmode == AllocFailStrategy::EXIT_OOM) {
+  if (size != 0 && p == NULL && alloc_failmode == AllocFailStrategy::EXIT_OOM) {

And let the caller handle the NULL pointer.

As David put it:
"I just don't like the allocate-1-byte approach"

/Robbin


On 02/04/2017 01: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_realloc_size_0/jdk10-webrev.01/webrev/
>
> For reference, the old discussion back in october 16:
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-October/021675.html
>
> 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Chris Plummer
In reply to this post by Dmitry Samersoff
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 <[hidden email]>
>> 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
>>>>
>>>>
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Robbin Ehn
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 <[hidden email]>
>>> 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 compari=son
>>>>> 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
>>>>>
>>>>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi all,

thanks for all your input, and sorry for the delay!

So, realloc. To summarize our discussion:

There seems to be a slight preference toward treating allocations with
size==0 as an error which should yield an assert. Both Chris and me see the
size=1 approach as a pragmatic way to deal with this situation. But I can
see both sides, so as a test I implemented another approach:

http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_
realloc_size_0/jdk10-webrev.02-alternate-version/webrev/

Now this is a very trivial change. I assert on size=0 for both malloc and
realloc. I decided to be more tolerant on the release build and return a
1byte sized block (yes, it is actually more) to the caller. Returning NULL
is a bad idea, because no-one checks the errno, everyone just assumes OOM.

I built slowdebug and ran it through hotspot jtreg tests on my Linux box.
It yielded ~80 asserts (for malloc(size=0), no assert for realloc(size=0))

They boil down to ~10 issues, all of which a variation of the same theme.
Coding wants to do something with a collection of n items - allocates an
array of size n for temporary data, iterates over the items, then frees the
array again. If n=0, code degenerates to one malloc, followed by free. Note
that the fact that n is 0 is often a result of the test itself testing
corner cases.

I honestly do not consider these occurrences errors. Yes, the
malloc()/free() could have been skipped, but on the other hand the
programmers may have relied knowingly on the established behavior of
os::malloc() returning a freeable pointer for size=0. I attempted to "fix"
some of the occurrences by skipping the malloc() if n was zero, but in all
cases the coding ended up less readable than before. (Sorry, I cannot post
this fix attempt, I accidentally deleted my patch).

Here are some examples of asserts:

--

1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosure::
G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
 34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_cset_indices()
const+0x1a8

Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices() is
called when G1CollectionSet::_collection_set_cur_length is zero. I assume
this is fine, verification could just be skipped at this point, so the fix
would be trivial.

--

gc/arguments/TestG1ConcRefinementThreads: VM is called with
-XX:G1ConcRefinementThreads=0. Then, we assert here:

 32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
NativeCallStack const&, AllocFa-XX:G1ConcRefinementThreads=
0ilStrategy::AllocFailEnum)+0x3b
 33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(CardTableEntryClosure*,
int*)+0x197
 34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
 35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62

Not sure at which layer one would handle this. If
-XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
setting not just be forbidden?

--
At a number of tests linux numa initialization failed:

 32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
 33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_allocate(int)+0x12f


 34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int, bool,
MemoryType)+0x65
 35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e

Here, a GrowableArray is initialized with size = 0. This seems to be an
allowed pattern, so GrowableArray would have to be fixed to delay the
malloc until the first real allocation.

--

So, options:

- we go this way - all the occurrences would have to be fixed, and
programmers would have to be made aware of the changed behavior of
os::malloc(size=0).
- we could only assert for realloc(size=0), but I find this even more
inconsistent than it is now.
- we could rethink the original decision.

More things to ponder:

- what about free(NULL), do we want to assert here too? On one hand, this
may indicate an error, possibly a double free(). On the other hand, passing
NULL to free() is even more a known established pattern than passing size=0
to malloc, and programmers may just do this expecting os::free() to have
the same semantics as ::free().

- this patch relies on the fact that we have control about who calls
os::malloc/os::free (similar to the decision to use malloc headers in both
NMT and os.cpp). Should we ever open up our malloc/free calls to the
outside world, we may loose this control and would have to make sure that
os::malloc/os::free behave like ::malloc/::free. Why would we open
os::malloc to the outside? For instance, to get NMT malloc coverage over
the JDK native libraries. Granted, a lot more would have to change to
enable this. (Note that we have a malloc statistic in place in our fork
which spans hotspot + native jdk, and we do this by calling os::malloc from
the jdk native code).


Kind Regards, Thomas




On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]> wrote:

> 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 <[hidden email]>
>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Robbin Ehn
Thanks for testing the assert approach.

Since you found malloc(0) is an "allowed pattern" I'm fine with the first patch.
We should add some comments about this in os.hpp

And thanks again for the investigation!

/Robbin

On 02/27/2017 12:20 PM, Thomas Stüfe wrote:

>I attempted to "fix" some of the occurrences by skipping the malloc() if n was zero, but
> in all cases the coding ended up less readable than before. (Sorry, I cannot post this fix attempt, I accidentally deleted my patch).
>
> Here are some examples of asserts:
>
> --
>
> 1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
>  34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_cset_indices() const+0x1a8
>
> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices() is called when G1CollectionSet::_collection_set_cur_length is zero. I assume this is fine,
> verification could just be skipped at this point, so the fix would be trivial.
>
> --
>
> gc/arguments/TestG1ConcRefinementThreads: VM is called with -XX:G1ConcRefinementThreads=0. Then, we assert here:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType, NativeCallStack const&, AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>
> Not sure at which layer one would handle this. If -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this setting not just be forbidden?
>
> --
> At a number of tests linux numa initialization failed:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType, NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_allocate(int)+0x12f
>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int, bool, MemoryType)+0x65
>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>
> Here, a GrowableArray is initialized with size = 0. This seems to be an allowed pattern, so GrowableArray would have to be fixed to delay the malloc until the first real
> allocation.
>
> --
>
> So, options:
>
> - we go this way - all the occurrences would have to be fixed, and programmers would have to be made aware of the changed behavior of os::malloc(size=0).
> - we could only assert for realloc(size=0), but I find this even more inconsistent than it is now.
> - we could rethink the original decision.
>
> More things to ponder:
>
> - what about free(NULL), do we want to assert here too? On one hand, this may indicate an error, possibly a double free(). On the other hand, passing NULL to free() is even
> more a known established pattern than passing size=0 to malloc, and programmers may just do this expecting os::free() to have the same semantics as ::free().
>
> - this patch relies on the fact that we have control about who calls os::malloc/os::free (similar to the decision to use malloc headers in both NMT and os.cpp). Should we
> ever open up our malloc/free calls to the outside world, we may loose this control and would have to make sure that os::malloc/os::free behave like ::malloc/::free. Why
> would we open os::malloc to the outside? For instance, to get NMT malloc coverage over the JDK native libraries. Granted, a lot more would have to change to enable this.
> (Note that we have a malloc statistic in place in our fork which spans hotspot + native jdk, and we do this by calling os::malloc from the jdk native code).
>
>
> Kind Regards, Thomas
>
>
>
>
> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email] <mailto:[hidden email]>> wrote:
>
>     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 <[hidden email] <mailto:[hidden email]>>
>                 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 <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 <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
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Thanks Robbin! I'll wait for the others to chime in before continuing work
on this.

Kind Regards, Thomas

On Tue, Feb 28, 2017 at 12:52 PM, Robbin Ehn <[hidden email]> wrote:

> Thanks for testing the assert approach.
>
> Since you found malloc(0) is an "allowed pattern" I'm fine with the first
> patch.
> We should add some comments about this in os.hpp
>
> And thanks again for the investigation!
>
> /Robbin
>
>
> On 02/27/2017 12:20 PM, Thomas Stüfe wrote:
>
>> I attempted to "fix" some of the occurrences by skipping the malloc() if
>> n was zero, but
>> in all cases the coding ended up less readable than before. (Sorry, I
>> cannot post this fix attempt, I accidentally deleted my patch).
>>
>> Here are some examples of asserts:
>>
>> --
>>
>> 1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosur
>> e::G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
>>  34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_cset_indices()
>> const+0x1a8
>>
>> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices()
>> is called when G1CollectionSet::_collection_set_cur_length is zero. I
>> assume this is fine,
>> verification could just be skipped at this point, so the fix would be
>> trivial.
>>
>> --
>>
>> gc/arguments/TestG1ConcRefinementThreads: VM is called with
>> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>
>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>> NativeCallStack const&, AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::
>> AllocFailEnum)+0x3b
>>  33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(CardTableEntryClosure*,
>> int*)+0x197
>>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>
>> Not sure at which layer one would handle this. If
>> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
>> setting not just be forbidden?
>>
>> --
>> At a number of tests linux numa initialization failed:
>>
>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>> NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>>  33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_allo
>> cate(int)+0x12f
>>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int,
>> bool, MemoryType)+0x65
>>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>>
>> Here, a GrowableArray is initialized with size = 0. This seems to be an
>> allowed pattern, so GrowableArray would have to be fixed to delay the
>> malloc until the first real
>> allocation.
>>
>> --
>>
>> So, options:
>>
>> - we go this way - all the occurrences would have to be fixed, and
>> programmers would have to be made aware of the changed behavior of
>> os::malloc(size=0).
>> - we could only assert for realloc(size=0), but I find this even more
>> inconsistent than it is now.
>> - we could rethink the original decision.
>>
>> More things to ponder:
>>
>> - what about free(NULL), do we want to assert here too? On one hand, this
>> may indicate an error, possibly a double free(). On the other hand, passing
>> NULL to free() is even
>> more a known established pattern than passing size=0 to malloc, and
>> programmers may just do this expecting os::free() to have the same
>> semantics as ::free().
>>
>> - this patch relies on the fact that we have control about who calls
>> os::malloc/os::free (similar to the decision to use malloc headers in both
>> NMT and os.cpp). Should we
>> ever open up our malloc/free calls to the outside world, we may loose
>> this control and would have to make sure that os::malloc/os::free behave
>> like ::malloc/::free. Why
>> would we open os::malloc to the outside? For instance, to get NMT malloc
>> coverage over the JDK native libraries. Granted, a lot more would have to
>> change to enable this.
>> (Note that we have a malloc statistic in place in our fork which spans
>> hotspot + native jdk, and we do this by calling os::malloc from the jdk
>> native code).
>>
>>
>> Kind Regards, Thomas
>>
>>
>>
>>
>> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     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 <
>> [hidden email] <mailto:[hidden email]>>
>>                 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/~st
>> uefe/webrevs/8168542-os_reallo <http://cr.openjdk.java.net/~s
>> tuefe/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/p
>> ipermail/hotspot-runtime-dev/2 <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
>>
>>
>>
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Dmitry Samersoff
In reply to this post by Thomas Stüfe-2
Thomas,

I agree that malloc behavior should be exactly the same as realloc one.
So if we can't assert size==0 for both, lets go with the first patch.

But IMHO, zero-size allocation is not a good pattern and we should fix
it if possible. Of course, not within scope of this fix.

I'm not sure wether NMT can help us to track zero allocations. After the
fix NMT will record 1 byte allocation instead.

free(NULL) is a different story and I don't think it's a problem.

-Dmitry

On 2017-02-27 14:20, Thomas Stüfe wrote:

> Hi all,
>
> thanks for all your input, and sorry for the delay!
>
> So, realloc. To summarize our discussion:
>
> There seems to be a slight preference toward treating allocations with
> size==0 as an error which should yield an assert. Both Chris and me see the
> size=1 approach as a pragmatic way to deal with this situation. But I can
> see both sides, so as a test I implemented another approach:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_
> realloc_size_0/jdk10-webrev.02-alternate-version/webrev/
>
> Now this is a very trivial change. I assert on size=0 for both malloc and
> realloc. I decided to be more tolerant on the release build and return a
> 1byte sized block (yes, it is actually more) to the caller. Returning NULL
> is a bad idea, because no-one checks the errno, everyone just assumes OOM.
>
> I built slowdebug and ran it through hotspot jtreg tests on my Linux box.
> It yielded ~80 asserts (for malloc(size=0), no assert for realloc(size=0))
>
> They boil down to ~10 issues, all of which a variation of the same theme.
> Coding wants to do something with a collection of n items - allocates an
> array of size n for temporary data, iterates over the items, then frees the
> array again. If n=0, code degenerates to one malloc, followed by free. Note
> that the fact that n is 0 is often a result of the test itself testing
> corner cases.
>
> I honestly do not consider these occurrences errors. Yes, the
> malloc()/free() could have been skipped, but on the other hand the
> programmers may have relied knowingly on the established behavior of
> os::malloc() returning a freeable pointer for size=0. I attempted to "fix"
> some of the occurrences by skipping the malloc() if n was zero, but in all
> cases the coding ended up less readable than before. (Sorry, I cannot post
> this fix attempt, I accidentally deleted my patch).
>
> Here are some examples of asserts:
>
> --
>
> 1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosure::
> G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
>  34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_cset_indices()
> const+0x1a8
>
> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices() is
> called when G1CollectionSet::_collection_set_cur_length is zero. I assume
> this is fine, verification could just be skipped at this point, so the fix
> would be trivial.
>
> --
>
> gc/arguments/TestG1ConcRefinementThreads: VM is called with
> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> NativeCallStack const&, AllocFa-XX:G1ConcRefinementThreads=
> 0ilStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(CardTableEntryClosure*,
> int*)+0x197
>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>
> Not sure at which layer one would handle this. If
> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
> setting not just be forbidden?
>
> --
> At a number of tests linux numa initialization failed:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_allocate(int)+0x12f
>
>
>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int, bool,
> MemoryType)+0x65
>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>
> Here, a GrowableArray is initialized with size = 0. This seems to be an
> allowed pattern, so GrowableArray would have to be fixed to delay the
> malloc until the first real allocation.
>
> --
>
> So, options:
>
> - we go this way - all the occurrences would have to be fixed, and
> programmers would have to be made aware of the changed behavior of
> os::malloc(size=0).
> - we could only assert for realloc(size=0), but I find this even more
> inconsistent than it is now.
> - we could rethink the original decision.
>
> More things to ponder:
>
> - what about free(NULL), do we want to assert here too? On one hand, this
> may indicate an error, possibly a double free(). On the other hand, passing
> NULL to free() is even more a known established pattern than passing size=0
> to malloc, and programmers may just do this expecting os::free() to have
> the same semantics as ::free().
>
> - this patch relies on the fact that we have control about who calls
> os::malloc/os::free (similar to the decision to use malloc headers in both
> NMT and os.cpp). Should we ever open up our malloc/free calls to the
> outside world, we may loose this control and would have to make sure that
> os::malloc/os::free behave like ::malloc/::free. Why would we open
> os::malloc to the outside? For instance, to get NMT malloc coverage over
> the JDK native libraries. Granted, a lot more would have to change to
> enable this. (Note that we have a malloc statistic in place in our fork
> which spans hotspot + native jdk, and we do this by calling os::malloc from
> the jdk native code).
>
>
> Kind Regards, Thomas
>
>
>
>
> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]> wrote:
>
>> 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 <[hidden email]>
>>>>> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi Dmitry,

On Tue, Feb 28, 2017 at 4:47 PM, Dmitry Samersoff <
[hidden email]> wrote:

> Thomas,
>
> I agree that malloc behavior should be exactly the same as realloc one.
> So if we can't assert size==0 for both, lets go with the first patch.
>
>
Thank you.


> But IMHO, zero-size allocation is not a good pattern and we should fix
> it if possible. Of course, not within scope of this fix.
>
> I'm not sure wether NMT can help us to track zero allocations. After the
> fix NMT will record 1 byte allocation instead.
>
>
Sure. Some ideas:

- add another MEMFLAG enum value like "size_0_allocation" and replace the
caller's memflag enum value with this one (a bit ugly but easy to do).

- a bit more elaborate: with size==0, do not allocate at all but return a
known sentinel pointer value, e.g. "0xABCDABCD". On os::free(), recognize
this sentinel value as a former size-0 allocation and just ignore it. NMT
gets the sentinel handed down as address and should recognize it too, just
counting size-0 allocations or printing the callstacks or keeping an extra
list of these callstacks... Added bonus: if the sentinel value points to
memory which is guaranteed to be invalid, we crash immediately if the
caller does access the memory (so, if his notion of the size is different
from 0, a real error).

free(NULL) is a different story and I don't think it's a problem.
>
>
Pretty sure I already saw free(NULL) in the wild which were the result of
an error - pointer freed was from a sparsely populated pointer array which
was accessed via a wrong index. But I think free(NULL) is used very often
in innocent situations, so I agree. I would not assert here.

Kind regards, Thomas


> -Dmitry
>
> On 2017-02-27 14:20, Thomas Stüfe wrote:
> > Hi all,
> >
> > thanks for all your input, and sorry for the delay!
> >
> > So, realloc. To summarize our discussion:
> >
> > There seems to be a slight preference toward treating allocations with
> > size==0 as an error which should yield an assert. Both Chris and me see
> the
> > size=1 approach as a pragmatic way to deal with this situation. But I can
> > see both sides, so as a test I implemented another approach:
> >
> > http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_
> > realloc_size_0/jdk10-webrev.02-alternate-version/webrev/
> >
> > Now this is a very trivial change. I assert on size=0 for both malloc and
> > realloc. I decided to be more tolerant on the release build and return a
> > 1byte sized block (yes, it is actually more) to the caller. Returning
> NULL
> > is a bad idea, because no-one checks the errno, everyone just assumes
> OOM.
> >
> > I built slowdebug and ran it through hotspot jtreg tests on my Linux box.
> > It yielded ~80 asserts (for malloc(size=0), no assert for
> realloc(size=0))
> >
> > They boil down to ~10 issues, all of which a variation of the same theme.
> > Coding wants to do something with a collection of n items - allocates an
> > array of size n for temporary data, iterates over the items, then frees
> the
> > array again. If n=0, code degenerates to one malloc, followed by free.
> Note
> > that the fact that n is 0 is often a result of the test itself testing
> > corner cases.
> >
> > I honestly do not consider these occurrences errors. Yes, the
> > malloc()/free() could have been skipped, but on the other hand the
> > programmers may have relied knowingly on the established behavior of
> > os::malloc() returning a freeable pointer for size=0. I attempted to
> "fix"
> > some of the occurrences by skipping the malloc() if n was zero, but in
> all
> > cases the coding ended up less readable than before. (Sorry, I cannot
> post
> > this fix attempt, I accidentally deleted my patch).
> >
> > Here are some examples of asserts:
> >
> > --
> >
> > 1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosure::
> > G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
> >  34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_
> cset_indices()
> > const+0x1a8
> >
> > Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices()
> is
> > called when G1CollectionSet::_collection_set_cur_length is zero. I
> assume
> > this is fine, verification could just be skipped at this point, so the
> fix
> > would be trivial.
> >
> > --
> >
> > gc/arguments/TestG1ConcRefinementThreads: VM is called with
> > -XX:G1ConcRefinementThreads=0. Then, we assert here:
> >
> >  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> > NativeCallStack const&, AllocFa-XX:G1ConcRefinementThreads=
> > 0ilStrategy::AllocFailEnum)+0x3b
> >  33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(
> CardTableEntryClosure*,
> > int*)+0x197
> >  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
> >  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
> >
> > Not sure at which layer one would handle this. If
> > -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
> > setting not just be forbidden?
> >
> > --
> > At a number of tests linux numa initialization failed:
> >
> >  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> > NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
> >  33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_
> allocate(int)+0x12f
> >
> >
> >  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int,
> bool,
> > MemoryType)+0x65
> >  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
> >
> > Here, a GrowableArray is initialized with size = 0. This seems to be an
> > allowed pattern, so GrowableArray would have to be fixed to delay the
> > malloc until the first real allocation.
> >
> > --
> >
> > So, options:
> >
> > - we go this way - all the occurrences would have to be fixed, and
> > programmers would have to be made aware of the changed behavior of
> > os::malloc(size=0).
> > - we could only assert for realloc(size=0), but I find this even more
> > inconsistent than it is now.
> > - we could rethink the original decision.
> >
> > More things to ponder:
> >
> > - what about free(NULL), do we want to assert here too? On one hand, this
> > may indicate an error, possibly a double free(). On the other hand,
> passing
> > NULL to free() is even more a known established pattern than passing
> size=0
> > to malloc, and programmers may just do this expecting os::free() to have
> > the same semantics as ::free().
> >
> > - this patch relies on the fact that we have control about who calls
> > os::malloc/os::free (similar to the decision to use malloc headers in
> both
> > NMT and os.cpp). Should we ever open up our malloc/free calls to the
> > outside world, we may loose this control and would have to make sure that
> > os::malloc/os::free behave like ::malloc/::free. Why would we open
> > os::malloc to the outside? For instance, to get NMT malloc coverage over
> > the JDK native libraries. Granted, a lot more would have to change to
> > enable this. (Note that we have a malloc statistic in place in our fork
> > which spans hotspot + native jdk, and we do this by calling os::malloc
> from
> > the jdk native code).
> >
> >
> > Kind Regards, Thomas
> >
> >
> >
> >
> > On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]>
> wrote:
> >
> >> 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 <
> [hidden email]>
> >>>>> 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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Thomas Stüfe-2
Hi Thomas,

Thanks for all the work you have done on this.

I'm disappointed that the assertions on size==0 yielded so many problem
areas. I think in many cases these do indicate actual bugs in the logic
- eg I'm not sure what a GrowableArray of size 0 is supposed to
represent! But not sure it is worth spending the cycles trying to
address this.

BTW the handling of G1ConcRefinementThreads=0 is definitely a bug in the
argument processing logic. 0 means to set ergonomically, so it should
either be handled thusly if explicitly set or else explicitly disallowed
- the code currently passes through the 0 as the number of threads!

I do not have an issue with free(NULL) as that has always been
well-defined and avoids checking the pointer twice when not-NULL. I
prefer to see:

free(p);

rather than:

if (p) free(p);

So ... I'll concede to go with the first patch.

Thanks,
David
-----

On 27/02/2017 9:20 PM, Thomas Stüfe wrote:

> Hi all,
>
> thanks for all your input, and sorry for the delay!
>
> So, realloc. To summarize our discussion:
>
> There seems to be a slight preference toward treating allocations with
> size==0 as an error which should yield an assert. Both Chris and me see
> the size=1 approach as a pragmatic way to deal with this situation. But
> I can see both sides, so as a test I implemented another approach:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.02-alternate-version/webrev/
> <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.02-alternate-version/webrev/>
>
> Now this is a very trivial change. I assert on size=0 for both malloc
> and realloc. I decided to be more tolerant on the release build and
> return a 1byte sized block (yes, it is actually more) to the caller.
> Returning NULL is a bad idea, because no-one checks the errno, everyone
> just assumes OOM.
>
> I built slowdebug and ran it through hotspot jtreg tests on my Linux
> box. It yielded ~80 asserts (for malloc(size=0), no assert for
> realloc(size=0))
>
> They boil down to ~10 issues, all of which a variation of the same
> theme. Coding wants to do something with a collection of n items -
> allocates an array of size n for temporary data, iterates over the
> items, then frees the array again. If n=0, code degenerates to one
> malloc, followed by free. Note that the fact that n is 0 is often a
> result of the test itself testing corner cases.
>
> I honestly do not consider these occurrences errors. Yes, the
> malloc()/free() could have been skipped, but on the other hand the
> programmers may have relied knowingly on the established behavior of
> os::malloc() returning a freeable pointer for size=0. I attempted to
> "fix" some of the occurrences by skipping the malloc() if n was zero,
> but in all cases the coding ended up less readable than before. (Sorry,
> I cannot post this fix attempt, I accidentally deleted my patch).
>
> Here are some examples of asserts:
>
> --
>
> 1) 33 V  [libjvm.so+0xb17625]
>  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(unsigned
> long)+0xbb
>  34 V  [libjvm.so+0xb16588]
>  G1CollectionSet::verify_young_cset_indices() const+0x1a8
>
> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices()
> is called when G1CollectionSet::_collection_set_cur_length is zero. I
> assume this is fine, verification could just be skipped at this point,
> so the fix would be trivial.
>
> --
>
> gc/arguments/TestG1ConcRefinementThreads: VM is called with
> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> NativeCallStack const&,
> AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0x954689]
>  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>
> Not sure at which layer one would handle this. If
> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
> setting not just be forbidden?
>
> --
> At a number of tests linux numa initialization failed:
>
>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
> NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>  33 V  [libjvm.so+0xbf7aff]
>  GenericGrowableArray::raw_allocate(int)+0x12f
>
>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int,
> bool, MemoryType)+0x65
>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>
> Here, a GrowableArray is initialized with size = 0. This seems to be an
> allowed pattern, so GrowableArray would have to be fixed to delay the
> malloc until the first real allocation.
>
> --
>
> So, options:
>
> - we go this way - all the occurrences would have to be fixed, and
> programmers would have to be made aware of the changed behavior of
> os::malloc(size=0).
> - we could only assert for realloc(size=0), but I find this even more
> inconsistent than it is now.
> - we could rethink the original decision.
>
> More things to ponder:
>
> - what about free(NULL), do we want to assert here too? On one hand,
> this may indicate an error, possibly a double free(). On the other hand,
> passing NULL to free() is even more a known established pattern than
> passing size=0 to malloc, and programmers may just do this expecting
> os::free() to have the same semantics as ::free().
>
> - this patch relies on the fact that we have control about who calls
> os::malloc/os::free (similar to the decision to use malloc headers in
> both NMT and os.cpp). Should we ever open up our malloc/free calls to
> the outside world, we may loose this control and would have to make sure
> that os::malloc/os::free behave like ::malloc/::free. Why would we open
> os::malloc to the outside? For instance, to get NMT malloc coverage over
> the JDK native libraries. Granted, a lot more would have to change to
> enable this. (Note that we have a malloc statistic in place in our fork
> which spans hotspot + native jdk, and we do this by calling os::malloc
> from the jdk native code).
>
>
> Kind Regards, Thomas
>
>
>
>
> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     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
>                 <[hidden email] <mailto:[hidden email]>>
>                 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
>                         <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
>                         <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
>
>
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
In reply to this post by Thomas Stüfe-2
Hi,

thank you all for the decision. I updated the webrev in place, no change,
just added the reviewers:

http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.01/

I'll need someone to sponsor this change.

Kind Regards, Thomas



On Wed, Mar 1, 2017 at 5:49 AM, Chris Plummer <[hidden email]>
wrote:

> Hi Thomas,
>
> "first patch" is malloc(0) is treated as malloc(1), right? If that's the
> case, you already know I'm in agreement. Glad to see others are also
> agreeing. Not pretty, but reasonable given the current state of things.
>
> cheers,
>
> Chris
>
>
> On 2/28/17 7:21 AM, Thomas Stüfe wrote:
>
> Thanks Robbin! I'll wait for the others to chime in before continuing work
> on this.
>
> Kind Regards, Thomas
>
> On Tue, Feb 28, 2017 at 12:52 PM, Robbin Ehn <[hidden email]>
> wrote:
>
>> Thanks for testing the assert approach.
>>
>> Since you found malloc(0) is an "allowed pattern" I'm fine with the first
>> patch.
>> We should add some comments about this in os.hpp
>>
>> And thanks again for the investigation!
>>
>> /Robbin
>>
>>
>> On 02/27/2017 12:20 PM, Thomas Stüfe wrote:
>>
>>> I attempted to "fix" some of the occurrences by skipping the malloc() if
>>> n was zero, but
>>> in all cases the coding ended up less readable than before. (Sorry, I
>>> cannot post this fix attempt, I accidentally deleted my patch).
>>>
>>> Here are some examples of asserts:
>>>
>>> --
>>>
>>> 1) 33 V  [libjvm.so+0xb17625]  G1VerifyYoungCSetIndicesClosur
>>> e::G1VerifyYoungCSetIndicesClosure(unsigned long)+0xbb
>>>  34 V  [libjvm.so+0xb16588]  G1CollectionSet::verify_young_cset_indices()
>>> const+0x1a8
>>>
>>> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices()
>>> is called when G1CollectionSet::_collection_set_cur_length is zero. I
>>> assume this is fine,
>>> verification could just be skipped at this point, so the fix would be
>>> trivial.
>>>
>>> --
>>>
>>> gc/arguments/TestG1ConcRefinementThreads: VM is called with
>>> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>>
>>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>>> NativeCallStack const&, AllocFa-XX:G1ConcRefinementThr
>>> eads=0ilStrategy::AllocFailEnum)+0x3b
>>>  33 V  [libjvm.so+0x954689]  ConcurrentG1Refine::create(CardTableEntryClosure*,
>>> int*)+0x197
>>>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>>>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>>
>>> Not sure at which layer one would handle this. If
>>> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
>>> setting not just be forbidden?
>>>
>>> --
>>> At a number of tests linux numa initialization failed:
>>>
>>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>>> NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>>>  33 V  [libjvm.so+0xbf7aff]  GenericGrowableArray::raw_allo
>>> cate(int)+0x12f
>>>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int,
>>> bool, MemoryType)+0x65
>>>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>>>
>>> Here, a GrowableArray is initialized with size = 0. This seems to be an
>>> allowed pattern, so GrowableArray would have to be fixed to delay the
>>> malloc until the first real
>>> allocation.
>>>
>>> --
>>>
>>> So, options:
>>>
>>> - we go this way - all the occurrences would have to be fixed, and
>>> programmers would have to be made aware of the changed behavior of
>>> os::malloc(size=0).
>>> - we could only assert for realloc(size=0), but I find this even more
>>> inconsistent than it is now.
>>> - we could rethink the original decision.
>>>
>>> More things to ponder:
>>>
>>> - what about free(NULL), do we want to assert here too? On one hand,
>>> this may indicate an error, possibly a double free(). On the other hand,
>>> passing NULL to free() is even
>>> more a known established pattern than passing size=0 to malloc, and
>>> programmers may just do this expecting os::free() to have the same
>>> semantics as ::free().
>>>
>>> - this patch relies on the fact that we have control about who calls
>>> os::malloc/os::free (similar to the decision to use malloc headers in both
>>> NMT and os.cpp). Should we
>>> ever open up our malloc/free calls to the outside world, we may loose
>>> this control and would have to make sure that os::malloc/os::free behave
>>> like ::malloc/::free. Why
>>> would we open os::malloc to the outside? For instance, to get NMT malloc
>>> coverage over the JDK native libraries. Granted, a lot more would have to
>>> change to enable this.
>>> (Note that we have a malloc statistic in place in our fork which spans
>>> hotspot + native jdk, and we do this by calling os::malloc from the jdk
>>> native code).
>>>
>>>
>>> Kind Regards, Thomas
>>>
>>>
>>>
>>>
>>> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>     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 <
>>> [hidden email] <mailto:[hidden email]>>
>>>                 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/~st
>>> uefe/webrevs/8168542-os_reallo <http://cr.openjdk.java.net/~s
>>> tuefe/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/p
>>> ipermail/hotspot-runtime-dev/2 <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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
In reply to this post by David Holmes
Hi David,

On Wed, Mar 1, 2017 at 5:11 AM, David Holmes <[hidden email]>
wrote:

> Hi Thomas,
>
> Thanks for all the work you have done on this.
>
> I'm disappointed that the assertions on size==0 yielded so many problem
> areas. I think in many cases these do indicate actual bugs in the logic -
> eg I'm not sure what a GrowableArray of size 0 is supposed to represent!
> But not sure it is worth spending the cycles trying to address this.
>

I'd say a GrowableArray with an initial size of 0 could make sense. Just a
placeholder for memory which is expected to be allocated later.


> BTW the handling of G1ConcRefinementThreads=0 is definitely a bug in the
> argument processing logic. 0 means to set ergonomically, so it should
> either be handled thusly if explicitly set or else explicitly disallowed -
> the code currently passes through the 0 as the number of threads!
>
> I do not have an issue with free(NULL) as that has always been
> well-defined and avoids checking the pointer twice when not-NULL. I prefer
> to see:
>
> free(p);
>
> rather than:
>
> if (p) free(p);
>
> So ... I'll concede to go with the first patch.
>
>
Thank you!

..Thomas


> Thanks,
> David
> -----
>
>
> On 27/02/2017 9:20 PM, Thomas Stüfe wrote:
>
>> Hi all,
>>
>> thanks for all your input, and sorry for the delay!
>>
>> So, realloc. To summarize our discussion:
>>
>> There seems to be a slight preference toward treating allocations with
>> size==0 as an error which should yield an assert. Both Chris and me see
>> the size=1 approach as a pragmatic way to deal with this situation. But
>> I can see both sides, so as a test I implemented another approach:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reallo
>> c_size_0/jdk10-webrev.02-alternate-version/webrev/
>> <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reall
>> oc_size_0/jdk10-webrev.02-alternate-version/webrev/>
>>
>> Now this is a very trivial change. I assert on size=0 for both malloc
>> and realloc. I decided to be more tolerant on the release build and
>> return a 1byte sized block (yes, it is actually more) to the caller.
>> Returning NULL is a bad idea, because no-one checks the errno, everyone
>> just assumes OOM.
>>
>> I built slowdebug and ran it through hotspot jtreg tests on my Linux
>> box. It yielded ~80 asserts (for malloc(size=0), no assert for
>> realloc(size=0))
>>
>> They boil down to ~10 issues, all of which a variation of the same
>> theme. Coding wants to do something with a collection of n items -
>> allocates an array of size n for temporary data, iterates over the
>> items, then frees the array again. If n=0, code degenerates to one
>> malloc, followed by free. Note that the fact that n is 0 is often a
>> result of the test itself testing corner cases.
>>
>> I honestly do not consider these occurrences errors. Yes, the
>> malloc()/free() could have been skipped, but on the other hand the
>> programmers may have relied knowingly on the established behavior of
>> os::malloc() returning a freeable pointer for size=0. I attempted to
>> "fix" some of the occurrences by skipping the malloc() if n was zero,
>> but in all cases the coding ended up less readable than before. (Sorry,
>> I cannot post this fix attempt, I accidentally deleted my patch).
>>
>> Here are some examples of asserts:
>>
>> --
>>
>> 1) 33 V  [libjvm.so+0xb17625]
>>  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClo
>> sure(unsigned
>> long)+0xbb
>>  34 V  [libjvm.so+0xb16588]
>>  G1CollectionSet::verify_young_cset_indices() const+0x1a8
>>
>> Happens in a lot of tests. G1CollectionSet::verify_young_cset_indices()
>> is called when G1CollectionSet::_collection_set_cur_length is zero. I
>> assume this is fine, verification could just be skipped at this point,
>> so the fix would be trivial.
>>
>> --
>>
>> gc/arguments/TestG1ConcRefinementThreads: VM is called with
>> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>
>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>> NativeCallStack const&,
>> AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>>  33 V  [libjvm.so+0x954689]
>>  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>
>> Not sure at which layer one would handle this. If
>> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should this
>> setting not just be forbidden?
>>
>> --
>> At a number of tests linux numa initialization failed:
>>
>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long, MemoryType,
>> NativeCallStack const&, AllocFailStrategy::AllocFailEnum)+0x3b
>>  33 V  [libjvm.so+0xbf7aff]
>>  GenericGrowableArray::raw_allocate(int)+0x12f
>>
>>  34 V  [libjvm.so+0x6b6b89]  GrowableArray<int>::GrowableArray(int,
>> bool, MemoryType)+0x65
>>  35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>>
>> Here, a GrowableArray is initialized with size = 0. This seems to be an
>> allowed pattern, so GrowableArray would have to be fixed to delay the
>> malloc until the first real allocation.
>>
>> --
>>
>> So, options:
>>
>> - we go this way - all the occurrences would have to be fixed, and
>> programmers would have to be made aware of the changed behavior of
>> os::malloc(size=0).
>> - we could only assert for realloc(size=0), but I find this even more
>> inconsistent than it is now.
>> - we could rethink the original decision.
>>
>> More things to ponder:
>>
>> - what about free(NULL), do we want to assert here too? On one hand,
>> this may indicate an error, possibly a double free(). On the other hand,
>> passing NULL to free() is even more a known established pattern than
>> passing size=0 to malloc, and programmers may just do this expecting
>> os::free() to have the same semantics as ::free().
>>
>> - this patch relies on the fact that we have control about who calls
>> os::malloc/os::free (similar to the decision to use malloc headers in
>> both NMT and os.cpp). Should we ever open up our malloc/free calls to
>> the outside world, we may loose this control and would have to make sure
>> that os::malloc/os::free behave like ::malloc/::free. Why would we open
>> os::malloc to the outside? For instance, to get NMT malloc coverage over
>> the JDK native libraries. Granted, a lot more would have to change to
>> enable this. (Note that we have a malloc statistic in place in our fork
>> which spans hotspot + native jdk, and we do this by calling os::malloc
>> from the jdk native code).
>>
>>
>> Kind Regards, Thomas
>>
>>
>>
>>
>> On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     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
>>                 <[hidden email] <mailto:[hidden email]
>> >>
>>
>>                 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/~st
>> uefe/webrevs/8168542-os_reallo
>>                         <http://cr.openjdk.java.net/~s
>> tuefe/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/p
>> ipermail/hotspot-runtime-dev/2
>>                         <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
>>
>>
>>
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Thomas Stüfe-2
I'll sponsor this for you Thomas.

David

On 1/03/2017 6:07 PM, Thomas Stüfe wrote:

> Hi,
>
> thank you all for the decision. I updated the webrev in place, no
> change, just added the reviewers:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_realloc_size_0/jdk10-webrev.01/
>
> I'll need someone to sponsor this change.
>
> Kind Regards, Thomas
>
>
>
> On Wed, Mar 1, 2017 at 5:49 AM, Chris Plummer <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi Thomas,
>
>     "first patch" is malloc(0) is treated as malloc(1), right? If that's
>     the case, you already know I'm in agreement. Glad to see others are
>     also agreeing. Not pretty, but reasonable given the current state of
>     things.
>
>     cheers,
>
>     Chris
>
>
>     On 2/28/17 7:21 AM, Thomas Stüfe wrote:
>>     Thanks Robbin! I'll wait for the others to chime in before
>>     continuing work on this.
>>
>>     Kind Regards, Thomas
>>
>>     On Tue, Feb 28, 2017 at 12:52 PM, Robbin Ehn
>>     <[hidden email] <mailto:[hidden email]>> wrote:
>>
>>         Thanks for testing the assert approach.
>>
>>         Since you found malloc(0) is an "allowed pattern" I'm fine
>>         with the first patch.
>>         We should add some comments about this in os.hpp
>>
>>         And thanks again for the investigation!
>>
>>         /Robbin
>>
>>
>>         On 02/27/2017 12:20 PM, Thomas Stüfe wrote:
>>
>>             I attempted to "fix" some of the occurrences by skipping
>>             the malloc() if n was zero, but
>>             in all cases the coding ended up less readable than
>>             before. (Sorry, I cannot post this fix attempt, I
>>             accidentally deleted my patch).
>>
>>             Here are some examples of asserts:
>>
>>             --
>>
>>             1) 33 V  [libjvm.so+0xb17625]
>>             G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(unsigned
>>             long)+0xbb
>>              34 V  [libjvm.so+0xb16588]
>>             G1CollectionSet::verify_young_cset_indices() const+0x1a8
>>
>>             Happens in a lot of tests.
>>             G1CollectionSet::verify_young_cset_indices() is called
>>             when G1CollectionSet::_collection_set_cur_length is zero.
>>             I assume this is fine,
>>             verification could just be skipped at this point, so the
>>             fix would be trivial.
>>
>>             --
>>
>>             gc/arguments/TestG1ConcRefinementThreads: VM is called
>>             with -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>
>>              32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long,
>>             MemoryType, NativeCallStack const&,
>>             AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>>              33 V  [libjvm.so+0x954689]
>>             ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>>              34 V  [libjvm.so+0xaf596a]
>>             G1CollectedHeap::initialize()+0x1a4
>>              35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>
>>             Not sure at which layer one would handle this. If
>>             -XX:G1ConcRefinementThreads=0 makes no sense as an option,
>>             should this setting not just be forbidden?
>>
>>             --
>>             At a number of tests linux numa initialization failed:
>>
>>              32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long,
>>             MemoryType, NativeCallStack const&,
>>             AllocFailStrategy::AllocFailEnum)+0x3b
>>              33 V  [libjvm.so+0xbf7aff]
>>             GenericGrowableArray::raw_allocate(int)+0x12f
>>              34 V  [libjvm.so+0x6b6b89]
>>             GrowableArray<int>::GrowableArray(int, bool, MemoryType)+0x65
>>              35 V  [libjvm.so+0x105b294]  os::Linux::libnuma_init()+0x17e
>>
>>             Here, a GrowableArray is initialized with size = 0. This
>>             seems to be an allowed pattern, so GrowableArray would
>>             have to be fixed to delay the malloc until the first real
>>             allocation.
>>
>>             --
>>
>>             So, options:
>>
>>             - we go this way - all the occurrences would have to be
>>             fixed, and programmers would have to be made aware of the
>>             changed behavior of os::malloc(size=0).
>>             - we could only assert for realloc(size=0), but I find
>>             this even more inconsistent than it is now.
>>             - we could rethink the original decision.
>>
>>             More things to ponder:
>>
>>             - what about free(NULL), do we want to assert here too? On
>>             one hand, this may indicate an error, possibly a double
>>             free(). On the other hand, passing NULL to free() is even
>>             more a known established pattern than passing size=0 to
>>             malloc, and programmers may just do this expecting
>>             os::free() to have the same semantics as ::free().
>>
>>             - this patch relies on the fact that we have control about
>>             who calls os::malloc/os::free (similar to the decision to
>>             use malloc headers in both NMT and os.cpp). Should we
>>             ever open up our malloc/free calls to the outside world,
>>             we may loose this control and would have to make sure that
>>             os::malloc/os::free behave like ::malloc/::free. Why
>>             would we open os::malloc to the outside? For instance, to
>>             get NMT malloc coverage over the JDK native libraries.
>>             Granted, a lot more would have to change to enable this.
>>             (Note that we have a malloc statistic in place in our fork
>>             which spans hotspot + native jdk, and we do this by
>>             calling os::malloc from the jdk native code).
>>
>>
>>             Kind Regards, Thomas
>>
>>
>>
>>
>>             On Wed, Feb 8, 2017 at 10:01 AM, Robbin Ehn
>>             <[hidden email] <mailto:[hidden email]>
>>             <mailto:[hidden email]
>>             <mailto:[hidden email]>>> wrote:
>>
>>                 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 <[hidden email]
>>             <mailto:[hidden email]>
>>             <mailto:[hidden email]
>>             <mailto:[hidden email]>>>
>>                             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>
>>             <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
>>             <http://cr.openjdk.java.net/%7Estuefe/webrevs/8168542-os_reallo>
>>             <http://cr.openjdk.java.net/~stuefe/webrevs/8168542-os_reallo
>>             <http://cr.openjdk.java.net/%7Estuefe/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
>>             <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2>
>>             <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2
>>             <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
>>
>>
>>
>>
>>
>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Thomas Schatzl
In reply to this post by David Holmes
Hi all,

  some comments:

On Wed, 2017-03-01 at 14:11 +1000, David Holmes wrote:

> Hi Thomas,
>
> Thanks for all the work you have done on this.
>
> I'm disappointed that the assertions on size==0 yielded so many
> problem areas. I think in many cases these do indicate actual bugs in
> the logic - eg I'm not sure what a GrowableArray of size 0 is
> supposed to represent! But not sure it is worth spending the cycles
> trying to address this.
>
> BTW the handling of G1ConcRefinementThreads=0 is definitely a bug in
> the argument processing logic. 0 means to set ergonomically, so it
> should either be handled thusly if explicitly set or else explicitly
> disallowed
> - the code currently passes through the 0 as the number of threads!

No. Explicitly setting it to zero means zero refinement threads as of
current.

Refinement is an optimization, as in "G1 does not absolutely need
refinement threads for operation", and this switch is part of the
incantation to disable them. The mutators will do the refinement
themselves.

There may be an inconsistency with the meaning of "0" when set
explicitly for this kind of threads compared to others.

>
> I do not have an issue with free(NULL) as that has always been 
> well-defined and avoids checking the pointer twice when not-NULL. I 
> prefer to see:
>
> free(p);
>
> rather than:
>
> if (p) free(p);
>
> So ... I'll concede to go with the first patch.
>
> Thanks,
> David
> -----
>
> On 27/02/2017 9:20 PM, Thomas Stüfe wrote:
> >
> > Hi all,
> >
> > thanks for all your input, and sorry for the delay!
> >
[...]

> > Here are some examples of asserts:
> >
> > --
> >
> > 1) 33 V  [libjvm.so+0xb17625]
> >  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(u
> > nsigned long)+0xbb 34 V  [libjvm.so+0xb16588]
> >  G1CollectionSet::verify_young_cset_indices() const+0x1a8
> >
> > Happens in a lot of tests.
> > G1CollectionSet::verify_young_cset_indices()
> > is called when G1CollectionSet::_collection_set_cur_length is zero.
> > I assume this is fine, verification could just be skipped at this
> > point, so the fix would be trivial.

Verification also does not do anything in this case, i.e. the code then
iterates over zero regions.

There is another related situation with G1RemSetSummary::initialize().
In case of zero refinement threads, the code also allocates a zero-
sized array to avoid special casing this situation throughout. The code
is correct.

> > --
> >
> > gc/arguments/TestG1ConcRefinementThreads: VM is called with
> > -XX:G1ConcRefinementThreads=0. Then, we assert here:
> >
> >  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long,
> > MemoryType,
> > NativeCallStack const&,
> > AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
> >  33 V  [libjvm.so+0x954689]
> >  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
> >  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
> >  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
> >
> > Not sure at which layer one would handle this. If
> > -XX:G1ConcRefinementThreads=0 makes no sense as an option, should
> > this setting not just be forbidden?

See above.

Imho (and just rambling without considering any consequences of an
implementation):

Somewhere in this thread there has been a comment to let malloc(0)
return a sentinel value that can be checked for - that seems to be most
preferable to me.
The reason for the malloc(0) call is that all of these violations seem
to be about allocating zero-sized arrays, and AllocateHeap et al. just
pass on whatever they get as input values. I do not see zero-sized
arrays to be something wrong.

Or just add a thin "array" abstraction in hotspot that allows that.

Practical considerations like catching potential errors or performance
might make that one undesired though.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
Hi Thomas,

On 6/03/2017 8:58 PM, Thomas Schatzl wrote:

> Hi all,
>
>   some comments:
>
> On Wed, 2017-03-01 at 14:11 +1000, David Holmes wrote:
>> Hi Thomas,
>>
>> Thanks for all the work you have done on this.
>>
>> I'm disappointed that the assertions on size==0 yielded so many
>> problem areas. I think in many cases these do indicate actual bugs in
>> the logic - eg I'm not sure what a GrowableArray of size 0 is
>> supposed to represent! But not sure it is worth spending the cycles
>> trying to address this.
>>
>> BTW the handling of G1ConcRefinementThreads=0 is definitely a bug in
>> the argument processing logic. 0 means to set ergonomically, so it
>> should either be handled thusly if explicitly set or else explicitly
>> disallowed
>> - the code currently passes through the 0 as the number of threads!
>
> No. Explicitly setting it to zero means zero refinement threads as of
> current.
>
> Refinement is an optimization, as in "G1 does not absolutely need
> refinement threads for operation", and this switch is part of the
> incantation to disable them. The mutators will do the refinement
> themselves.
>
> There may be an inconsistency with the meaning of "0" when set
> explicitly for this kind of threads compared to others.

It is very bad form to have 0 as a default mean one thing, and 0 as an
explicit value meaning something else IMHO. If 0 is a legal value then
-1, or some other value should have been chosen to mean "use ergonomics".

David
-----

>>
>> I do not have an issue with free(NULL) as that has always been
>> well-defined and avoids checking the pointer twice when not-NULL. I
>> prefer to see:
>>
>> free(p);
>>
>> rather than:
>>
>> if (p) free(p);
>>
>> So ... I'll concede to go with the first patch.
>>
>> Thanks,
>> David
>> -----
>>
>> On 27/02/2017 9:20 PM, Thomas Stüfe wrote:
>>>
>>> Hi all,
>>>
>>> thanks for all your input, and sorry for the delay!
>>>
> [...]
>>> Here are some examples of asserts:
>>>
>>> --
>>>
>>> 1) 33 V  [libjvm.so+0xb17625]
>>>  G1VerifyYoungCSetIndicesClosure::G1VerifyYoungCSetIndicesClosure(u
>>> nsigned long)+0xbb 34 V  [libjvm.so+0xb16588]
>>>  G1CollectionSet::verify_young_cset_indices() const+0x1a8
>>>
>>> Happens in a lot of tests.
>>> G1CollectionSet::verify_young_cset_indices()
>>> is called when G1CollectionSet::_collection_set_cur_length is zero.
>>> I assume this is fine, verification could just be skipped at this
>>> point, so the fix would be trivial.
>
> Verification also does not do anything in this case, i.e. the code then
> iterates over zero regions.
>
> There is another related situation with G1RemSetSummary::initialize().
> In case of zero refinement threads, the code also allocates a zero-
> sized array to avoid special casing this situation throughout. The code
> is correct.
>
>>> --
>>>
>>> gc/arguments/TestG1ConcRefinementThreads: VM is called with
>>> -XX:G1ConcRefinementThreads=0. Then, we assert here:
>>>
>>>  32 V  [libjvm.so+0x5adeee]  AllocateHeap(unsigned long,
>>> MemoryType,
>>> NativeCallStack const&,
>>> AllocFa-XX:G1ConcRefinementThreads=0ilStrategy::AllocFailEnum)+0x3b
>>>  33 V  [libjvm.so+0x954689]
>>>  ConcurrentG1Refine::create(CardTableEntryClosure*, int*)+0x197
>>>  34 V  [libjvm.so+0xaf596a]  G1CollectedHeap::initialize()+0x1a4
>>>  35 V  [libjvm.so+0x127e940]  Universe::initialize_heap()+0x62
>>>
>>> Not sure at which layer one would handle this. If
>>> -XX:G1ConcRefinementThreads=0 makes no sense as an option, should
>>> this setting not just be forbidden?
>
> See above.
>
> Imho (and just rambling without considering any consequences of an
> implementation):
>
> Somewhere in this thread there has been a comment to let malloc(0)
> return a sentinel value that can be checked for - that seems to be most
> preferable to me.
> The reason for the malloc(0) call is that all of these violations seem
> to be about allocating zero-sized arrays, and AllocateHeap et al. just
> pass on whatever they get as input values. I do not see zero-sized
> arrays to be something wrong.
>
> Or just add a thin "array" abstraction in hotspot that allows that.
>
> Practical considerations like catching potential errors or performance
> might make that one undesired though.
>
> Thanks,
>   Thomas
>
12
Loading...