diff mbox series

[FFmpeg-devel] avutil/buffer: Avoid allocation of AVBuffer when using buffer pool

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
Related show

Checks

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

Commit Message

Andreas Rheinhardt Sept. 14, 2021, 10:16 a.m. UTC
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(-)

Comments

Andreas Rheinhardt Sept. 17, 2021, 2:56 a.m. UTC | #1
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
James Almer Sept. 17, 2021, 5:02 p.m. UTC | #2
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 {
>
Andreas Rheinhardt Sept. 17, 2021, 5:44 p.m. UTC | #3
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 mbox series

Patch

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 {