On Mon, 9 Apr 2012, Ronald S. Bultje wrote:
Hi,
On Mon, Apr 9, 2012 at 4:50 AM, Martin Storsjö <[email protected]> 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
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
