mem: Consistently return NULL for av_malloc(0)

Message ID 1333896707-11392-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö April 8, 2012, 2:51 p.m.
Plain POSIX malloc(0) is allowed to return either NULL or a
non-NULL pointer. The calling code should be ready to handle
a NULL return as a correct return (instead of a failure) if the size
to allocate was 0 - this makes sure the condition is handled
in a consistent way across platforms.

This also avoids calling posix_memalign(&ptr, 32, 0) on OS X,
which returns an invalid pointer (a non-NULL pointer that causes
crashes when passed to av_free).
---
 libavutil/mem.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Martin Storsjö April 9, 2012, 11:50 a.m. | #1
On Sun, 8 Apr 2012, Martin Storsjö wrote:

> Plain POSIX malloc(0) is allowed to return either NULL or a
> non-NULL pointer. The calling code should be ready to handle
> a NULL return as a correct return (instead of a failure) if the size
> to allocate was 0 - this makes sure the condition is handled
> in a consistent way across platforms.
>
> This also avoids calling posix_memalign(&ptr, 32, 0) on OS X,
> which returns an invalid pointer (a non-NULL pointer that causes
> crashes when passed to av_free).
> ---
> libavutil/mem.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavutil/mem.c b/libavutil/mem.c
> index b6230cf..43fe3f6 100644
> --- a/libavutil/mem.c
> +++ b/libavutil/mem.c
> @@ -69,7 +69,7 @@ void *av_malloc(size_t size)
> #endif
>
>     /* let's disallow possible ambiguous cases */
> -    if(size > (INT_MAX-32) )
> +    if (size > (INT_MAX-32) || !size)
>         return NULL;
>
> #if CONFIG_MEMALIGN_HACK
> -- 
> 1.7.9.4

So, what do others think about this?

Ronald thinks we should add abort() for the size==0 case in debug builds, 
personally I'm a bit undecided. (Returning NULL in non-debug builds seems 
to be agreed on at least, since it's a more controlled behaviour compared 
to crashing.)

There are quite a few cases with code like this:

array = av_mallocz(count*sizeof(*array));
if (!array)
     return AVERROR(ENOMEM);
for (i = 0; i < count; i++)
     array[i] = do_something(i);
av_free(array);

Since malloc may return NULL for size==0, the allocation at least needs to 
be expanded to:

array = av_mallocz(count*sizeof(*array));
if (!array && count)
     return AVERROR(ENOMEM);

If we add the abort to that case, we need to either change it into:

array = count ? av_mallocz(count*sizeof(*array)) : NULL;
if (!array && count)
     return AVERROR(ENOMEM);

Or add an if clause around the whole allocation/use block.

One upside to adding the abort is that it is much easier to track down 
compared to code that suddenly fails due to returning AVERROR(ENOMEM) in 
some case.

// Martin
Ronald Bultje April 9, 2012, 2:01 p.m. | #2
Hi,

On Mon, Apr 9, 2012 at 4:50 AM, Martin Storsjö <martin@martin.st> wrote:
> On Sun, 8 Apr 2012, Martin Storsjö wrote:
>
>> Plain POSIX malloc(0) is allowed to return either NULL or a
>> non-NULL pointer. The calling code should be ready to handle
>> a NULL return as a correct return (instead of a failure) if the size
>> to allocate was 0 - this makes sure the condition is handled
>> in a consistent way across platforms.
>>
>> This also avoids calling posix_memalign(&ptr, 32, 0) on OS X,
>> which returns an invalid pointer (a non-NULL pointer that causes
>> crashes when passed to av_free).
>> ---
>> libavutil/mem.c |    2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>> index b6230cf..43fe3f6 100644
>> --- a/libavutil/mem.c
>> +++ b/libavutil/mem.c
>> @@ -69,7 +69,7 @@ void *av_malloc(size_t size)
>> #endif
>>
>>    /* let's disallow possible ambiguous cases */
>> -    if(size > (INT_MAX-32) )
>> +    if (size > (INT_MAX-32) || !size)
>>        return NULL;
>>
>> #if CONFIG_MEMALIGN_HACK
>> --
>> 1.7.9.4
>
>
> So, what do others think about this?
>
> Ronald thinks we should add abort() for the size==0 case in debug builds,
> personally I'm a bit undecided. (Returning NULL in non-debug builds seems to
> be agreed on at least, since it's a more controlled behaviour compared to
> crashing.)
>
> There are quite a few cases with code like this:
>
> array = av_mallocz(count*sizeof(*array));
> if (!array)
>    return AVERROR(ENOMEM);
> for (i = 0; i < count; i++)
>    array[i] = do_something(i);
> av_free(array);
>
> Since malloc may return NULL for size==0, the allocation at least needs to
> be expanded to:
>
> array = av_mallocz(count*sizeof(*array));
> if (!array && count)
>    return AVERROR(ENOMEM);
>
> If we add the abort to that case, we need to either change it into:
>
> array = count ? av_mallocz(count*sizeof(*array)) : NULL;
> if (!array && count)
>    return AVERROR(ENOMEM);
>
> Or add an if clause around the whole allocation/use block.

Right, you need the branch anyway, so I'd recommend an if around the
whole block.

> One upside to adding the abort is that it is much easier to track down
> compared to code that suddenly fails due to returning AVERROR(ENOMEM) in
> some case.

That's indeed why I'm so strongly in favour of it. Right now only Mac
behaves differently. This makes it consistent again.

Ronald
Luca Barbato April 9, 2012, 5:23 p.m. | #3
On 09/04/12 07:01, Ronald S. Bultje wrote:
> That's indeed why I'm so strongly in favour of it. Right now only Mac
> behaves differently. This makes it consistent again.
> 
> Ronald

fine for me.


lu
Martin Storsjö April 9, 2012, 5:34 p.m. | #4
On Mon, 9 Apr 2012, Ronald S. Bultje wrote:

> Hi,
>
> On Mon, Apr 9, 2012 at 4:50 AM, Martin Storsjö <martin@martin.st> wrote:
>> On Sun, 8 Apr 2012, Martin Storsjö wrote:
>>
>>> Plain POSIX malloc(0) is allowed to return either NULL or a
>>> non-NULL pointer. The calling code should be ready to handle
>>> a NULL return as a correct return (instead of a failure) if the size
>>> to allocate was 0 - this makes sure the condition is handled
>>> in a consistent way across platforms.
>>>
>>> This also avoids calling posix_memalign(&ptr, 32, 0) on OS X,
>>> which returns an invalid pointer (a non-NULL pointer that causes
>>> crashes when passed to av_free).
>>> ---
>>> libavutil/mem.c |    2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>> index b6230cf..43fe3f6 100644
>>> --- a/libavutil/mem.c
>>> +++ b/libavutil/mem.c
>>> @@ -69,7 +69,7 @@ void *av_malloc(size_t size)
>>> #endif
>>>
>>>    /* let's disallow possible ambiguous cases */
>>> -    if(size > (INT_MAX-32) )
>>> +    if (size > (INT_MAX-32) || !size)
>>>        return NULL;
>>>
>>> #if CONFIG_MEMALIGN_HACK
>>> --
>>> 1.7.9.4
>>
>>
>> So, what do others think about this?
>>
>> Ronald thinks we should add abort() for the size==0 case in debug builds,
>> personally I'm a bit undecided. (Returning NULL in non-debug builds seems to
>> be agreed on at least, since it's a more controlled behaviour compared to
>> crashing.)
>>
>> There are quite a few cases with code like this:
>>
>> array = av_mallocz(count*sizeof(*array));
>> if (!array)
>>    return AVERROR(ENOMEM);
>> for (i = 0; i < count; i++)
>>    array[i] = do_something(i);
>> av_free(array);
>>
>> Since malloc may return NULL for size==0, the allocation at least needs to
>> be expanded to:
>>
>> array = av_mallocz(count*sizeof(*array));
>> if (!array && count)
>>    return AVERROR(ENOMEM);
>>
>> If we add the abort to that case, we need to either change it into:
>>
>> array = count ? av_mallocz(count*sizeof(*array)) : NULL;
>> if (!array && count)
>>    return AVERROR(ENOMEM);
>>
>> Or add an if clause around the whole allocation/use block.
>
> Right, you need the branch anyway, so I'd recommend an if around the
> whole block.

That adds one indentation level - I prefer how the first one looks. That's 
all of course just a bikeshed discussion.

>> One upside to adding the abort is that it is much easier to track down
>> compared to code that suddenly fails due to returning AVERROR(ENOMEM) in
>> some case.
>
> That's indeed why I'm so strongly in favour of it. Right now only Mac
> behaves differently. This makes it consistent again.

It becomes consistent already by making it return NULL - the abort just 
helps tracking down issues, and excludes the first pattern.

// Martin
Ronald Bultje April 9, 2012, 5:59 p.m. | #5
Hi,

On Mon, Apr 9, 2012 at 10:34 AM, Martin Storsjö <martin@martin.st> wrote:
> On Mon, 9 Apr 2012, Ronald S. Bultje wrote:
>> On Mon, Apr 9, 2012 at 4:50 AM, Martin Storsjö <martin@martin.st> wrote:
>>> On Sun, 8 Apr 2012, Martin Storsjö wrote:
>>>> Plain POSIX malloc(0) is allowed to return either NULL or a
>>>> non-NULL pointer. The calling code should be ready to handle
>>>> a NULL return as a correct return (instead of a failure) if the size
>>>> to allocate was 0 - this makes sure the condition is handled
>>>> in a consistent way across platforms.
>>>>
>>>> This also avoids calling posix_memalign(&ptr, 32, 0) on OS X,
>>>> which returns an invalid pointer (a non-NULL pointer that causes
>>>> crashes when passed to av_free).
>>>> ---
>>>> libavutil/mem.c |    2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavutil/mem.c b/libavutil/mem.c
>>>> index b6230cf..43fe3f6 100644
>>>> --- a/libavutil/mem.c
>>>> +++ b/libavutil/mem.c
>>>> @@ -69,7 +69,7 @@ void *av_malloc(size_t size)
>>>> #endif
>>>>
>>>>    /* let's disallow possible ambiguous cases */
>>>> -    if(size > (INT_MAX-32) )
>>>> +    if (size > (INT_MAX-32) || !size)
>>>>        return NULL;
>>>>
>>>> #if CONFIG_MEMALIGN_HACK
>>>> --
>>>> 1.7.9.4
>>>
>>>
>>>
>>> So, what do others think about this?
>>>
>>> Ronald thinks we should add abort() for the size==0 case in debug builds,
>>> personally I'm a bit undecided. (Returning NULL in non-debug builds seems
>>> to
>>> be agreed on at least, since it's a more controlled behaviour compared to
>>> crashing.)
>>>
>>> There are quite a few cases with code like this:
>>>
>>> array = av_mallocz(count*sizeof(*array));
>>> if (!array)
>>>    return AVERROR(ENOMEM);
>>> for (i = 0; i < count; i++)
>>>    array[i] = do_something(i);
>>> av_free(array);
>>>
>>> Since malloc may return NULL for size==0, the allocation at least needs
>>> to
>>> be expanded to:
>>>
>>> array = av_mallocz(count*sizeof(*array));
>>> if (!array && count)
>>>    return AVERROR(ENOMEM);
>>>
>>> If we add the abort to that case, we need to either change it into:
>>>
>>> array = count ? av_mallocz(count*sizeof(*array)) : NULL;
>>> if (!array && count)
>>>    return AVERROR(ENOMEM);
>>>
>>> Or add an if clause around the whole allocation/use block.
>>
>>
>> Right, you need the branch anyway, so I'd recommend an if around the
>> whole block.
>
>
> That adds one indentation level - I prefer how the first one looks. That's
> all of course just a bikeshed discussion.
>
>
>>> One upside to adding the abort is that it is much easier to track down
>>> compared to code that suddenly fails due to returning AVERROR(ENOMEM) in
>>> some case.
>>
>>
>> That's indeed why I'm so strongly in favour of it. Right now only Mac
>> behaves differently. This makes it consistent again.
>
>
> It becomes consistent already by making it return NULL - the abort just
> helps tracking down issues, and excludes the first pattern.

Fair enough, let's go your way (just to get over this), abort on debug
and return null on non-debug is OK.

Ronald

Patch

diff --git a/libavutil/mem.c b/libavutil/mem.c
index b6230cf..43fe3f6 100644
--- a/libavutil/mem.c
+++ b/libavutil/mem.c
@@ -69,7 +69,7 @@  void *av_malloc(size_t size)
 #endif
 
     /* let's disallow possible ambiguous cases */
-    if(size > (INT_MAX-32) )
+    if (size > (INT_MAX-32) || !size)
         return NULL;
 
 #if CONFIG_MEMALIGN_HACK