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. > 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 _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
