[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 at 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 at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>