[FFmpeg-devel] [PATCH 2/2] avutil/buffer: reject NULL as argument for the av_buffer_pool_init2() alloc callback

James Almer jamrial at gmail.com
Mon Jun 1 04:30:00 EEST 2020


On 5/31/2020 7:30 AM, Nicolas George wrote:
> James Almer (12020-05-30):
>> This prevents NULL pointer dereference crashes when calling av_buffer_pool_get()
>> using the resulting pool.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavutil/buffer.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/buffer.c b/libavutil/buffer.c
>> index 6d9cb7428e..6fe8f19c39 100644
>> --- a/libavutil/buffer.c
>> +++ b/libavutil/buffer.c
>> @@ -220,7 +220,11 @@ AVBufferPool *av_buffer_pool_init2(int size, void *opaque,
>>                                     AVBufferRef* (*alloc)(void *opaque, int size),
>>                                     void (*pool_free)(void *opaque))
>>  {
>> -    AVBufferPool *pool = av_mallocz(sizeof(*pool));
>> +    AVBufferPool *pool;
>> +
>> +    if (!alloc)
>> +        return NULL;
>> +    pool = av_mallocz(sizeof(*pool));
>>      if (!pool)
>>          return NULL;
> 
> I do not like this: this function can return NULL for AVERROR(ENOMEM),
> but now it also means "idiot programmer thought NULL was a valid
> callback".
> 
> The error code to "idiot programmer did something stupid and should have
> read the doc" should be SIGABORT. Proper error return should be reserved
> for cases that cannot be tested statically.
> 
> So, in this case:
> 
> 	av_assert0(alloc);
> 
> If the code is tested, it is perfectly equivalent anyway, because alloc
> will not be NULL.
> 
> Regards,

I guess replacing one crash with another earlier crash is not too bad
(Although i dislike asserts used to catch invalid arguments), but we
can't chalk it up to "idiot user that didn't read the docs" seeing how
in some functions certain parameters may be NULL and it's undocumented
(see patch 1/2 from this same set).

An alternative is doing the same as av_buffer_pool_init() and falling
back to using the default allocator if none is passed, but that's a
change in behavior (From not working to working, instead of from
crashing to crashing earlier).


More information about the ffmpeg-devel mailing list