[FFmpeg-devel] [PATCH 3/4] dict.c: Free non-strduped av_dict_set arguments on error.

[wm4]

On Mon, 11 Aug 2014 21:17:18 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> Unfortunately this was not explicitly documented and thus
> might be very risky.
> But basically all uses I saw in FFmpeg had a memleak in these
> cases.

It's the more convenient behavior, although on the other hand it feels
wrong to change the input data on error.

This makes me wonder, isn't AV_DICT_DONT_STRDUP_* too obscure and too
much of a microoptimization, that we have to risk retro-guessing these
subtle semantics?

(But the patch is probably OK, IMHO.)

> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> ---
>  libavutil/dict.c | 9 +++++++--
>  libavutil/dict.h | 2 ++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index 9fdc6d6..f23b768 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -91,7 +91,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>          AVDictionaryEntry *tmp = av_realloc(m->elems,
>                                              (m->count + 1) * sizeof(*m->elems));
>          if (!tmp)
> -            return AVERROR(ENOMEM);
> +            goto err_out;
>          m->elems = tmp;
>      }
>      if (value) {
> @@ -105,7 +105,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>              int len = strlen(oldval) + strlen(value) + 1;
>              char *newval = av_mallocz(len);
>              if (!newval)
> -                return AVERROR(ENOMEM);
> +                goto err_out;
>              av_strlcat(newval, oldval, len);
>              av_freep(&oldval);
>              av_strlcat(newval, value, len);
> @@ -120,6 +120,11 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>      }
>  
>      return 0;
> +
> +err_out:
> +    if (flags & AV_DICT_DONT_STRDUP_KEY) av_free(key);
> +    if (flags & AV_DICT_DONT_STRDUP_VAL) av_free(value);
> +    return AVERROR(ENOMEM);
>  }
>  
>  int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value,
> diff --git a/libavutil/dict.h b/libavutil/dict.h
> index 06f1621..5e319fe 100644
> --- a/libavutil/dict.h
> +++ b/libavutil/dict.h
> @@ -115,6 +115,8 @@ int av_dict_count(const AVDictionary *m);
>  /**
>   * Set the given entry in *pm, overwriting an existing entry.
>   *
> + * Note: On error non-av_strduped arguments will be freed.
> + *

That doesn't really explain anything. Suggestion: mention
AV_DICT_DONT_STRDUP_* explicitly, and mention that the corresponding
argument will always be freed, even on error.

>   * @param pm pointer to a pointer to a dictionary struct. If *pm is NULL
>   * a dictionary struct is allocated and put in *pm.
>   * @param key entry key to add to *pm (will be av_strduped depending on flags)