[FFmpeg-devel] [PATCH 1/3] avutil/dict: Fix memleak when using AV_DICT_APPEND

[FFmpeg-devel] [PATCH 1/3] avutil/dict: Fix memleak when using AV_DICT_APPEND

[Andreas Rheinhardt]

If a key already exists in an AVDictionary and the AV_DICT_APPEND flag
is set, the old entry is at first discarded from the dictionary, but
a pointer to the value is kept. Lateron enough memory to store the
appended string is allocated; should this allocation fail, the old string
is not freed and hence leaks. This commit changes this by moving
creating the combined value to an earlier point in the function,
which also ensures that the AVDictionaty is unchanged in case of errors.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This is an improved version of https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191111011259.27313-1-andreas.rheinhardt@gmail.com/

 libavutil/dict.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/libavutil/dict.c b/libavutil/dict.c
index a4f638a1fc..c01165634e 100644
--- a/libavutil/dict.c
+++ b/libavutil/dict.c
@@ -73,7 +73,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
 {
     AVDictionary *m = *pm;
     AVDictionaryEntry *tag = NULL;
-    char *oldval = NULL, *copy_key = NULL, *copy_value = NULL;
+    char *copy_key = NULL, *copy_value = NULL;
 
     if (!(flags & AV_DICT_MULTIKEY)) {
         tag = av_dict_get(m, key, NULL, flags);
@@ -97,10 +97,17 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
             av_free(copy_value);
             return 0;
         }
-        if (flags & AV_DICT_APPEND)
-            oldval = tag->value;
-        else
-            av_free(tag->value);
+        if (copy_value && flags & AV_DICT_APPEND) {
+            size_t len = strlen(tag->value) + strlen(copy_value) + 1;
+            char *newval = av_mallocz(len);
+            if (!newval)
+                goto err_out;
+            av_strlcat(newval, tag->value, len);
+            av_strlcat(newval, copy_value, len);
+            av_freep(&copy_value);
+            copy_value = newval;
+        }
+        av_free(tag->value);
         av_free(tag->key);
         *tag = m->elems[--m->count];
     } else if (copy_value) {
@@ -113,17 +120,6 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
     if (copy_value) {
         m->elems[m->count].key = copy_key;
         m->elems[m->count].value = copy_value;
-        if (oldval && flags & AV_DICT_APPEND) {
-            size_t len = strlen(oldval) + strlen(copy_value) + 1;
-            char *newval = av_mallocz(len);
-            if (!newval)
-                goto err_out;
-            av_strlcat(newval, oldval, len);
-            av_freep(&oldval);
-            av_strlcat(newval, copy_value, len);
-            m->elems[m->count].value = newval;
-            av_freep(&copy_value);
-        }
         m->count++;
     } else {
         av_freep(&copy_key);
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/3] avutil/dict: Improve appending values

[Andreas Rheinhardt]

When appending two values (due to AV_DICT_APPEND), the earlier code
would first zero-allocate a buffer of the required size and then
copy both parts into it via av_strlcat(). This is problematic,
as it leads to quadratic performance in case of frequent enlargements.
Fix this by using av_realloc() (which is hopefully designed to handle
such cases in a better way than simply throwing the buffer we already
have away) and by copying the string via memcpy() (after all, we already
calculated the strlen of both strings).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/dict.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/libavutil/dict.c b/libavutil/dict.c
index c01165634e..1968063b0b 100644
--- a/libavutil/dict.c
+++ b/libavutil/dict.c
@@ -98,16 +98,17 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
             return 0;
         }
         if (copy_value && flags & AV_DICT_APPEND) {
-            size_t len = strlen(tag->value) + strlen(copy_value) + 1;
-            char *newval = av_mallocz(len);
+            size_t oldlen = strlen(tag->value);
+            size_t new_part_len = strlen(copy_value);
+            size_t len = oldlen + new_part_len + 1;
+            char *newval = av_realloc(tag->value, len);
             if (!newval)
                 goto err_out;
-            av_strlcat(newval, tag->value, len);
-            av_strlcat(newval, copy_value, len);
+            memcpy(newval + oldlen, copy_value, new_part_len + 1);
             av_freep(&copy_value);
             copy_value = newval;
-        }
-        av_free(tag->value);
+        } else
+            av_free(tag->value);
         av_free(tag->key);
         *tag = m->elems[--m->count];
     } else if (copy_value) {
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/3] avutil/dict: Avoid check whose result is known in advance

[Andreas Rheinhardt]

We know that an AVDictionary is not empty if we have just added
an entry to it, so only check for it being empty on the branch
that does not do so.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/dict.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavutil/dict.c b/libavutil/dict.c
index 1968063b0b..4bba041d0a 100644
--- a/libavutil/dict.c
+++ b/libavutil/dict.c
@@ -123,12 +123,12 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
         m->elems[m->count].value = copy_value;
         m->count++;
     } else {
+        if (!m->count) {
+            av_freep(&m->elems);
+            av_freep(pm);
+        }
         av_freep(&copy_key);
     }
-    if (!m->count) {
-        av_freep(&m->elems);
-        av_freep(pm);
-    }
 
     return 0;
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] avutil/dict: Avoid check whose result is known in advance

[Paul B Mahol]

On 9/13/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> We know that an AVDictionary is not empty if we have just added
> an entry to it, so only check for it being empty on the branch
> that does not do so.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/dict.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>

LGTM

> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index 1968063b0b..4bba041d0a 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -123,12 +123,12 @@ int av_dict_set(AVDictionary **pm, const char *key,
> const char *value,
>          m->elems[m->count].value = copy_value;
>          m->count++;
>      } else {
> +        if (!m->count) {
> +            av_freep(&m->elems);
> +            av_freep(pm);
> +        }
>          av_freep(&copy_key);
>      }
> -    if (!m->count) {
> -        av_freep(&m->elems);
> -        av_freep(pm);
> -    }
>
>      return 0;
>
> --
> 2.34.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/3] avutil/dict: Fix memleak when using AV_DICT_APPEND

[Paul B Mahol]

On 9/13/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> If a key already exists in an AVDictionary and the AV_DICT_APPEND flag
> is set, the old entry is at first discarded from the dictionary, but
> a pointer to the value is kept. Lateron enough memory to store the
> appended string is allocated; should this allocation fail, the old string
> is not freed and hence leaks. This commit changes this by moving
> creating the combined value to an earlier point in the function,
> which also ensures that the AVDictionaty is unchanged in case of errors.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This is an improved version of
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20191111011259.27313-1-andreas.rheinhardt@gmail.com/
>
>  libavutil/dict.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index a4f638a1fc..c01165634e 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -73,7 +73,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const
> char *value,
>  {
>      AVDictionary *m = *pm;
>      AVDictionaryEntry *tag = NULL;
> -    char *oldval = NULL, *copy_key = NULL, *copy_value = NULL;
> +    char *copy_key = NULL, *copy_value = NULL;
>
>      if (!(flags & AV_DICT_MULTIKEY)) {
>          tag = av_dict_get(m, key, NULL, flags);
> @@ -97,10 +97,17 @@ int av_dict_set(AVDictionary **pm, const char *key,
> const char *value,
>              av_free(copy_value);
>              return 0;
>          }
> -        if (flags & AV_DICT_APPEND)
> -            oldval = tag->value;
> -        else
> -            av_free(tag->value);
> +        if (copy_value && flags & AV_DICT_APPEND) {
> +            size_t len = strlen(tag->value) + strlen(copy_value) + 1;
> +            char *newval = av_mallocz(len);
> +            if (!newval)
> +                goto err_out;
> +            av_strlcat(newval, tag->value, len);
> +            av_strlcat(newval, copy_value, len);
> +            av_freep(&copy_value);
> +            copy_value = newval;
> +        }
> +        av_free(tag->value);
>          av_free(tag->key);
>          *tag = m->elems[--m->count];
>      } else if (copy_value) {
> @@ -113,17 +120,6 @@ int av_dict_set(AVDictionary **pm, const char *key,
> const char *value,
>      if (copy_value) {
>          m->elems[m->count].key = copy_key;
>          m->elems[m->count].value = copy_value;
> -        if (oldval && flags & AV_DICT_APPEND) {
> -            size_t len = strlen(oldval) + strlen(copy_value) + 1;
> -            char *newval = av_mallocz(len);
> -            if (!newval)
> -                goto err_out;
> -            av_strlcat(newval, oldval, len);
> -            av_freep(&oldval);
> -            av_strlcat(newval, copy_value, len);
> -            m->elems[m->count].value = newval;
> -            av_freep(&copy_value);
> -        }
>          m->count++;
>      } else {
>          av_freep(&copy_key);
> --
> 2.34.1
>

LGTM

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/3] avutil/dict: Improve appending values

[Paul B Mahol]

On 9/13/22, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> When appending two values (due to AV_DICT_APPEND), the earlier code
> would first zero-allocate a buffer of the required size and then
> copy both parts into it via av_strlcat(). This is problematic,
> as it leads to quadratic performance in case of frequent enlargements.
> Fix this by using av_realloc() (which is hopefully designed to handle
> such cases in a better way than simply throwing the buffer we already
> have away) and by copying the string via memcpy() (after all, we already
> calculated the strlen of both strings).
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavutil/dict.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index c01165634e..1968063b0b 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -98,16 +98,17 @@ int av_dict_set(AVDictionary **pm, const char *key,
> const char *value,
>              return 0;
>          }
>          if (copy_value && flags & AV_DICT_APPEND) {
> -            size_t len = strlen(tag->value) + strlen(copy_value) + 1;
> -            char *newval = av_mallocz(len);
> +            size_t oldlen = strlen(tag->value);
> +            size_t new_part_len = strlen(copy_value);
> +            size_t len = oldlen + new_part_len + 1;
> +            char *newval = av_realloc(tag->value, len);
>              if (!newval)
>                  goto err_out;
> -            av_strlcat(newval, tag->value, len);
> -            av_strlcat(newval, copy_value, len);
> +            memcpy(newval + oldlen, copy_value, new_part_len + 1);
>              av_freep(&copy_value);
>              copy_value = newval;
> -        }
> -        av_free(tag->value);
> +        } else
> +            av_free(tag->value);
>          av_free(tag->key);
>          *tag = m->elems[--m->count];
>      } else if (copy_value) {
> --
> 2.34.1
>


LGTM

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".