| Message ID | AM7PR03MB6660DC19DA1A3B34F432112E8FDA9@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 4e0da7d3117bbf84203358940cba82ef10b6dfde |
| Headers | show |
| Series | [FFmpeg-devel] avutil/buffer: Avoid allocation of AVBuffer when using buffer pool | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Andreas Rheinhardt: > Do this by putting an AVBuffer structure into BufferPoolEntry and > reuse it for all subsequent uses of said BufferPoolEntry. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/buffer.c | 44 +++++++++++++++++++++++++------------ > libavutil/buffer_internal.h | 11 ++++++++++ > 2 files changed, 41 insertions(+), 14 deletions(-) > > diff --git a/libavutil/buffer.c b/libavutil/buffer.c > index b13eeadffb..4d9ccf74b7 100644 > --- a/libavutil/buffer.c > +++ b/libavutil/buffer.c > @@ -26,16 +26,11 @@ > #include "mem.h" > #include "thread.h" > > -AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > - void (*free)(void *opaque, uint8_t *data), > - void *opaque, int flags) > +static AVBufferRef *buffer_create(AVBuffer *buf, uint8_t *data, size_t size, > + void (*free)(void *opaque, uint8_t *data), > + void *opaque, int flags) > { > AVBufferRef *ref = NULL; > - AVBuffer *buf = NULL; > - > - buf = av_mallocz(sizeof(*buf)); > - if (!buf) > - return NULL; > > buf->data = data; > buf->size = size; > @@ -47,10 +42,8 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > buf->flags = flags; > > ref = av_mallocz(sizeof(*ref)); > - if (!ref) { > - av_freep(&buf); > + if (!ref) > return NULL; > - } > > ref->buffer = buf; > ref->data = data; > @@ -59,6 +52,23 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > return ref; > } > > +AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > + void (*free)(void *opaque, uint8_t *data), > + void *opaque, int flags) > +{ > + AVBufferRef *ret; > + AVBuffer *buf = av_mallocz(sizeof(*buf)); > + if (!buf) > + return NULL; > + > + ret = buffer_create(buf, data, size, free, opaque, flags); > + if (!ret) { > + av_free(buf); > + return NULL; > + } > + return ret; > +} > + > void av_buffer_default_free(void *opaque, uint8_t *data) > { > av_free(data); > @@ -117,8 +127,12 @@ static void buffer_replace(AVBufferRef **dst, AVBufferRef **src) > av_freep(dst); > > if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) { > + /* b->free below might already free the structure containing *b, > + * so we have to read the flag now to avoid use-after-free. */ > + int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE); > b->free(b->opaque, b->data); > - av_freep(&b); > + if (free_avbuffer) > + av_free(b); > } > } > > @@ -378,11 +392,13 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool) > ff_mutex_lock(&pool->mutex); > buf = pool->pool; > if (buf) { > - ret = av_buffer_create(buf->data, pool->size, pool_release_buffer, > - buf, 0); > + memset(&buf->buffer, 0, sizeof(buf->buffer)); > + ret = buffer_create(&buf->buffer, buf->data, pool->size, > + pool_release_buffer, buf, 0); > if (ret) { > pool->pool = buf->next; > buf->next = NULL; > + buf->buffer.flags_internal |= BUFFER_FLAG_NO_FREE; > } > } else { > ret = pool_alloc_buffer(pool); > diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h > index 839dc05f8f..bdff1b5b32 100644 > --- a/libavutil/buffer_internal.h > +++ b/libavutil/buffer_internal.h > @@ -30,6 +30,11 @@ > * The buffer was av_realloc()ed, so it is reallocatable. > */ > #define BUFFER_FLAG_REALLOCATABLE (1 << 0) > +/** > + * The AVBuffer structure is part of a larger structure > + * and should not be freed. > + */ > +#define BUFFER_FLAG_NO_FREE (1 << 1) > > struct AVBuffer { > uint8_t *data; /**< data described by this buffer */ > @@ -73,6 +78,12 @@ typedef struct BufferPoolEntry { > > AVBufferPool *pool; > struct BufferPoolEntry *next; > + > + /* > + * An AVBuffer structure to (re)use as AVBuffer for subsequent uses > + * of this BufferPoolEntry. > + */ > + AVBuffer buffer; > } BufferPoolEntry; > > struct AVBufferPool { > Will apply this tomorrow unless there are objections. - Andreas
On 9/14/2021 7:16 AM, Andreas Rheinhardt wrote: > Do this by putting an AVBuffer structure into BufferPoolEntry and > reuse it for all subsequent uses of said BufferPoolEntry. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavutil/buffer.c | 44 +++++++++++++++++++++++++------------ > libavutil/buffer_internal.h | 11 ++++++++++ > 2 files changed, 41 insertions(+), 14 deletions(-) > > diff --git a/libavutil/buffer.c b/libavutil/buffer.c > index b13eeadffb..4d9ccf74b7 100644 > --- a/libavutil/buffer.c > +++ b/libavutil/buffer.c > @@ -26,16 +26,11 @@ > #include "mem.h" > #include "thread.h" > > -AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > - void (*free)(void *opaque, uint8_t *data), > - void *opaque, int flags) > +static AVBufferRef *buffer_create(AVBuffer *buf, uint8_t *data, size_t size, > + void (*free)(void *opaque, uint8_t *data), > + void *opaque, int flags) > { > AVBufferRef *ref = NULL; > - AVBuffer *buf = NULL; > - > - buf = av_mallocz(sizeof(*buf)); > - if (!buf) > - return NULL; > > buf->data = data; > buf->size = size; > @@ -47,10 +42,8 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > buf->flags = flags; > > ref = av_mallocz(sizeof(*ref)); > - if (!ref) { > - av_freep(&buf); > + if (!ref) > return NULL; > - } > > ref->buffer = buf; > ref->data = data; > @@ -59,6 +52,23 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > return ref; > } > > +AVBufferRef *av_buffer_create(uint8_t *data, size_t size, > + void (*free)(void *opaque, uint8_t *data), > + void *opaque, int flags) > +{ > + AVBufferRef *ret; > + AVBuffer *buf = av_mallocz(sizeof(*buf)); > + if (!buf) > + return NULL; > + > + ret = buffer_create(buf, data, size, free, opaque, flags); > + if (!ret) { > + av_free(buf); > + return NULL; > + } > + return ret; > +} > + > void av_buffer_default_free(void *opaque, uint8_t *data) > { > av_free(data); > @@ -117,8 +127,12 @@ static void buffer_replace(AVBufferRef **dst, AVBufferRef **src) > av_freep(dst); > > if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) { > + /* b->free below might already free the structure containing *b, > + * so we have to read the flag now to avoid use-after-free. */ > + int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE); > b->free(b->opaque, b->data); > - av_freep(&b); > + if (free_avbuffer) > + av_free(b); > } > } > > @@ -378,11 +392,13 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool) > ff_mutex_lock(&pool->mutex); > buf = pool->pool; > if (buf) { > - ret = av_buffer_create(buf->data, pool->size, pool_release_buffer, > - buf, 0); > + memset(&buf->buffer, 0, sizeof(buf->buffer)); > + ret = buffer_create(&buf->buffer, buf->data, pool->size, > + pool_release_buffer, buf, 0); > if (ret) { > pool->pool = buf->next; > buf->next = NULL; > + buf->buffer.flags_internal |= BUFFER_FLAG_NO_FREE; > } > } else { > ret = pool_alloc_buffer(pool); > diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h > index 839dc05f8f..bdff1b5b32 100644 > --- a/libavutil/buffer_internal.h > +++ b/libavutil/buffer_internal.h > @@ -30,6 +30,11 @@ > * The buffer was av_realloc()ed, so it is reallocatable. > */ > #define BUFFER_FLAG_REALLOCATABLE (1 << 0) > +/** > + * The AVBuffer structure is part of a larger structure > + * and should not be freed. > + */ > +#define BUFFER_FLAG_NO_FREE (1 << 1) How about calling it something more generic BUFFER_FLAG_POOL or BUFFER_FLAG_FROM_POOL, to signal that the buffer was allocated by the buffer pool API? It can be reused, like in av_buffer_pool_buffer_get_opaque() to ensure there will be no crashes if the user passes it an AVBufferRef that was not created by the buffer pool API, and instead return NULL. > > struct AVBuffer { > uint8_t *data; /**< data described by this buffer */ > @@ -73,6 +78,12 @@ typedef struct BufferPoolEntry { > > AVBufferPool *pool; > struct BufferPoolEntry *next; > + > + /* > + * An AVBuffer structure to (re)use as AVBuffer for subsequent uses > + * of this BufferPoolEntry. > + */ > + AVBuffer buffer; > } BufferPoolEntry; > > struct AVBufferPool { >
James Almer: > On 9/14/2021 7:16 AM, Andreas Rheinhardt wrote: >> Do this by putting an AVBuffer structure into BufferPoolEntry and >> reuse it for all subsequent uses of said BufferPoolEntry. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavutil/buffer.c | 44 +++++++++++++++++++++++++------------ >> libavutil/buffer_internal.h | 11 ++++++++++ >> 2 files changed, 41 insertions(+), 14 deletions(-) >> >> diff --git a/libavutil/buffer.c b/libavutil/buffer.c >> index b13eeadffb..4d9ccf74b7 100644 >> --- a/libavutil/buffer.c >> +++ b/libavutil/buffer.c >> @@ -26,16 +26,11 @@ >> #include "mem.h" >> #include "thread.h" >> -AVBufferRef *av_buffer_create(uint8_t *data, size_t size, >> - void (*free)(void *opaque, uint8_t *data), >> - void *opaque, int flags) >> +static AVBufferRef *buffer_create(AVBuffer *buf, uint8_t *data, >> size_t size, >> + void (*free)(void *opaque, uint8_t >> *data), >> + void *opaque, int flags) >> { >> AVBufferRef *ref = NULL; >> - AVBuffer *buf = NULL; >> - >> - buf = av_mallocz(sizeof(*buf)); >> - if (!buf) >> - return NULL; >> buf->data = data; >> buf->size = size; >> @@ -47,10 +42,8 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t >> size, >> buf->flags = flags; >> ref = av_mallocz(sizeof(*ref)); >> - if (!ref) { >> - av_freep(&buf); >> + if (!ref) >> return NULL; >> - } >> ref->buffer = buf; >> ref->data = data; >> @@ -59,6 +52,23 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t >> size, >> return ref; >> } >> +AVBufferRef *av_buffer_create(uint8_t *data, size_t size, >> + void (*free)(void *opaque, uint8_t *data), >> + void *opaque, int flags) >> +{ >> + AVBufferRef *ret; >> + AVBuffer *buf = av_mallocz(sizeof(*buf)); >> + if (!buf) >> + return NULL; >> + >> + ret = buffer_create(buf, data, size, free, opaque, flags); >> + if (!ret) { >> + av_free(buf); >> + return NULL; >> + } >> + return ret; >> +} >> + >> void av_buffer_default_free(void *opaque, uint8_t *data) >> { >> av_free(data); >> @@ -117,8 +127,12 @@ static void buffer_replace(AVBufferRef **dst, >> AVBufferRef **src) >> av_freep(dst); >> if (atomic_fetch_sub_explicit(&b->refcount, 1, >> memory_order_acq_rel) == 1) { >> + /* b->free below might already free the structure containing *b, >> + * so we have to read the flag now to avoid use-after-free. */ >> + int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE); >> b->free(b->opaque, b->data); >> - av_freep(&b); >> + if (free_avbuffer) >> + av_free(b); >> } >> } >> @@ -378,11 +392,13 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool >> *pool) >> ff_mutex_lock(&pool->mutex); >> buf = pool->pool; >> if (buf) { >> - ret = av_buffer_create(buf->data, pool->size, >> pool_release_buffer, >> - buf, 0); >> + memset(&buf->buffer, 0, sizeof(buf->buffer)); >> + ret = buffer_create(&buf->buffer, buf->data, pool->size, >> + pool_release_buffer, buf, 0); >> if (ret) { >> pool->pool = buf->next; >> buf->next = NULL; >> + buf->buffer.flags_internal |= BUFFER_FLAG_NO_FREE; >> } >> } else { >> ret = pool_alloc_buffer(pool); >> diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h >> index 839dc05f8f..bdff1b5b32 100644 >> --- a/libavutil/buffer_internal.h >> +++ b/libavutil/buffer_internal.h >> @@ -30,6 +30,11 @@ >> * The buffer was av_realloc()ed, so it is reallocatable. >> */ >> #define BUFFER_FLAG_REALLOCATABLE (1 << 0) >> +/** >> + * The AVBuffer structure is part of a larger structure >> + * and should not be freed. >> + */ >> +#define BUFFER_FLAG_NO_FREE (1 << 1) > > How about calling it something more generic BUFFER_FLAG_POOL or > BUFFER_FLAG_FROM_POOL, to signal that the buffer was allocated by the > buffer pool API? It can be reused, like in > av_buffer_pool_buffer_get_opaque() to ensure there will be no crashes if > the user passes it an AVBufferRef that was not created by the buffer > pool API, and instead return NULL. > The buffer pool API deals with two kinds of buffers: Those that are implicitly given to it by the user callbacks via an AVBufferRef (when there is no free pool member) and those that it created itself using already available buffers from the pool. This patch will only set this flag on the latter, as the allocation can not be avoided for the former. Yet both of these types can legitimately be used in av_buffer_pool_buffer_get_opaque(). So your usage would either need a new flag or I would have to copy the buffer returned from the user callback into BufferPoolEntry, set the flag and free the AVBuffer manually. I don't like the second approach, because this flag as-is can also be used to avoid the allocations of other buffers in libavutil (where we are allowed to use sizeof(AVBuffer)). (Actually we could put the AVBuffer in front of every buffer allocated by us; we just need to ensure that the guaranteed alignment of the buffer as seen by the user is maintained. Only buffers directly created via av_buffer_create() would still need to be independently allocated.) - Andreas >> struct AVBuffer { >> uint8_t *data; /**< data described by this buffer */ >> @@ -73,6 +78,12 @@ typedef struct BufferPoolEntry { >> AVBufferPool *pool; >> struct BufferPoolEntry *next; >> + >> + /* >> + * An AVBuffer structure to (re)use as AVBuffer for subsequent uses >> + * of this BufferPoolEntry. >> + */ >> + AVBuffer buffer; >> } BufferPoolEntry; >> struct AVBufferPool { >>
diff --git a/libavutil/buffer.c b/libavutil/buffer.c index b13eeadffb..4d9ccf74b7 100644 --- a/libavutil/buffer.c +++ b/libavutil/buffer.c @@ -26,16 +26,11 @@ #include "mem.h" #include "thread.h" -AVBufferRef *av_buffer_create(uint8_t *data, size_t size, - void (*free)(void *opaque, uint8_t *data), - void *opaque, int flags) +static AVBufferRef *buffer_create(AVBuffer *buf, uint8_t *data, size_t size, + void (*free)(void *opaque, uint8_t *data), + void *opaque, int flags) { AVBufferRef *ref = NULL; - AVBuffer *buf = NULL; - - buf = av_mallocz(sizeof(*buf)); - if (!buf) - return NULL; buf->data = data; buf->size = size; @@ -47,10 +42,8 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, buf->flags = flags; ref = av_mallocz(sizeof(*ref)); - if (!ref) { - av_freep(&buf); + if (!ref) return NULL; - } ref->buffer = buf; ref->data = data; @@ -59,6 +52,23 @@ AVBufferRef *av_buffer_create(uint8_t *data, size_t size, return ref; } +AVBufferRef *av_buffer_create(uint8_t *data, size_t size, + void (*free)(void *opaque, uint8_t *data), + void *opaque, int flags) +{ + AVBufferRef *ret; + AVBuffer *buf = av_mallocz(sizeof(*buf)); + if (!buf) + return NULL; + + ret = buffer_create(buf, data, size, free, opaque, flags); + if (!ret) { + av_free(buf); + return NULL; + } + return ret; +} + void av_buffer_default_free(void *opaque, uint8_t *data) { av_free(data); @@ -117,8 +127,12 @@ static void buffer_replace(AVBufferRef **dst, AVBufferRef **src) av_freep(dst); if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) { + /* b->free below might already free the structure containing *b, + * so we have to read the flag now to avoid use-after-free. */ + int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE); b->free(b->opaque, b->data); - av_freep(&b); + if (free_avbuffer) + av_free(b); } } @@ -378,11 +392,13 @@ AVBufferRef *av_buffer_pool_get(AVBufferPool *pool) ff_mutex_lock(&pool->mutex); buf = pool->pool; if (buf) { - ret = av_buffer_create(buf->data, pool->size, pool_release_buffer, - buf, 0); + memset(&buf->buffer, 0, sizeof(buf->buffer)); + ret = buffer_create(&buf->buffer, buf->data, pool->size, + pool_release_buffer, buf, 0); if (ret) { pool->pool = buf->next; buf->next = NULL; + buf->buffer.flags_internal |= BUFFER_FLAG_NO_FREE; } } else { ret = pool_alloc_buffer(pool); diff --git a/libavutil/buffer_internal.h b/libavutil/buffer_internal.h index 839dc05f8f..bdff1b5b32 100644 --- a/libavutil/buffer_internal.h +++ b/libavutil/buffer_internal.h @@ -30,6 +30,11 @@ * The buffer was av_realloc()ed, so it is reallocatable. */ #define BUFFER_FLAG_REALLOCATABLE (1 << 0) +/** + * The AVBuffer structure is part of a larger structure + * and should not be freed. + */ +#define BUFFER_FLAG_NO_FREE (1 << 1) struct AVBuffer { uint8_t *data; /**< data described by this buffer */ @@ -73,6 +78,12 @@ typedef struct BufferPoolEntry { AVBufferPool *pool; struct BufferPoolEntry *next; + + /* + * An AVBuffer structure to (re)use as AVBuffer for subsequent uses + * of this BufferPoolEntry. + */ + AVBuffer buffer; } BufferPoolEntry; struct AVBufferPool {
Do this by putting an AVBuffer structure into BufferPoolEntry and reuse it for all subsequent uses of said BufferPoolEntry. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavutil/buffer.c | 44 +++++++++++++++++++++++++------------ libavutil/buffer_internal.h | 11 ++++++++++ 2 files changed, 41 insertions(+), 14 deletions(-)