Hi, On Mon, Apr 9, 2012 at 10:34 AM, Martin Storsjö <mar...@martin.st> wrote: > On Mon, 9 Apr 2012, Ronald S. Bultje wrote: >> On Mon, Apr 9, 2012 at 4:50 AM, Martin Storsjö <mar...@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 _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
