diff mbox series

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

Message ID 20200530233204.683-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avutil/buffer: add a mention that some arguments from av_buffer_pool_init2() may be NULL
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer May 30, 2020, 11:32 p.m. UTC
This prevents NULL pointer dereference crashes when calling av_buffer_pool_get()
using the resulting pool.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Nicolas George May 31, 2020, 10:30 a.m. UTC | #1
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@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,
James Almer June 1, 2020, 1:30 a.m. UTC | #2
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@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).
Anton Khirnov June 1, 2020, 7:49 a.m. UTC | #3
Quoting James Almer (2020-06-01 03:30:00)
> 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@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).

I don't see that as a problem. Changing behaviour from something being
invalid to something being valid is okay, because nobody should have
done the invalid thing before and if they did it's their own problem.
Just bump the version so that people can check if the new behaviour is
available.

And this seems like the preferable option to me.

Also, if you do this then av_assert0(alloc || alloc2) in
pool_alloc_buffer() seems appropriate.
diff mbox series

Patch

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;