| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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,
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).
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 --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;
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(-)