[FFmpeg-devel] [PATCH] avutil/dict: Error out in case of key == NULL

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Up until now, using NULL as key in av_dict_get() on a non-empty
> AVDictionary would crash; using NULL as key in av_dict_set()
> would also crash for a non-empty AVDictionary unless AV_DICT_MULTIKEY
> was set; in case the dictionary was initially empty or AV_DICT_MULTIKEY
> was set, it was even possible for av_dict_set() to succeed when
> adding a NULL key, namely when one uses a value != NULL and
> the AV_DICT_DONT_STRDUP_VAL flag. Using av_dict_get() on such
> an AVDictionary will usually lead to crashes, though.
> 
> Fix this by actually checking for key in both functions; error out
> if they are NULL.
> 
> While just at it, also stop relying on av_strdup(NULL) to return NULL
> in av_dict_set().
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> ---
>  libavutil/dict.c       | 27 +++++++++++++++++----------
>  libavutil/tests/dict.c | 12 +++++++++---
>  2 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/libavutil/dict.c b/libavutil/dict.c
> index 4bba041d0a..14ad780a79 100644
> --- a/libavutil/dict.c
> +++ b/libavutil/dict.c
> @@ -43,7 +43,7 @@ AVDictionaryEntry *av_dict_get(const AVDictionary *m, const char *key,
>  {
>      unsigned int i, j;
>  
> -    if (!m)
> +    if (!m || !key)
>          return NULL;
>  
>      if (prev)
> @@ -74,7 +74,16 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>      AVDictionary *m = *pm;
>      AVDictionaryEntry *tag = NULL;
>      char *copy_key = NULL, *copy_value = NULL;
> +    int err;
>  
> +    if (flags & AV_DICT_DONT_STRDUP_VAL)
> +        copy_value = (void *)value;
> +    else if (value)
> +        copy_value = av_strdup(value);
> +    if (!key) {
> +        err = AVERROR(EINVAL);
> +        goto err_out;
> +    }
>      if (!(flags & AV_DICT_MULTIKEY)) {
>          tag = av_dict_get(m, key, NULL, flags);
>      }
> @@ -82,14 +91,10 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>          copy_key = (void *)key;
>      else
>          copy_key = av_strdup(key);
> -    if (flags & AV_DICT_DONT_STRDUP_VAL)
> -        copy_value = (void *)value;
> -    else if (copy_key)
> -        copy_value = av_strdup(value);
>      if (!m)
>          m = *pm = av_mallocz(sizeof(*m));
> -    if (!m || (key && !copy_key) || (value && !copy_value))
> -        goto err_out;
> +    if (!m || !copy_key || (value && !copy_value))
> +        goto enomem;
>  
>      if (tag) {
>          if (flags & AV_DICT_DONT_OVERWRITE) {
> @@ -103,7 +108,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>              size_t len = oldlen + new_part_len + 1;
>              char *newval = av_realloc(tag->value, len);
>              if (!newval)
> -                goto err_out;
> +                goto enomem;
>              memcpy(newval + oldlen, copy_value, new_part_len + 1);
>              av_freep(&copy_value);
>              copy_value = newval;
> @@ -115,7 +120,7 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>          AVDictionaryEntry *tmp = av_realloc_array(m->elems,
>                                                    m->count + 1, sizeof(*m->elems));
>          if (!tmp)
> -            goto err_out;
> +            goto enomem;
>          m->elems = tmp;
>      }
>      if (copy_value) {
> @@ -132,6 +137,8 @@ int av_dict_set(AVDictionary **pm, const char *key, const char *value,
>  
>      return 0;
>  
> +enomem:
> +    err = AVERROR(ENOMEM);
>  err_out:
>      if (m && !m->count) {
>          av_freep(&m->elems);
> @@ -139,7 +146,7 @@ err_out:
>      }
>      av_free(copy_key);
>      av_free(copy_value);
> -    return AVERROR(ENOMEM);
> +    return err;
>  }
>  
>  int av_dict_set_int(AVDictionary **pm, const char *key, int64_t value,
> diff --git a/libavutil/tests/dict.c b/libavutil/tests/dict.c
> index 56e98557a7..d053545f4d 100644
> --- a/libavutil/tests/dict.c
> +++ b/libavutil/tests/dict.c
> @@ -91,14 +91,20 @@ int main(void)
>      av_dict_set(&dict, "f", NULL, 0);
>      av_dict_set(&dict, "ff", "f", 0);
>      av_dict_set(&dict, "ff", "f", AV_DICT_APPEND);
> +    if (av_dict_get(dict, NULL, NULL, 0))
> +        printf("av_dict_get() does not correctly handle NULL key.\n");
>      e = NULL;
>      while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
>          printf("%s %s\n", e->key, e->value);
>      av_dict_free(&dict);
>  
> -    av_dict_set(&dict, NULL, "a", 0);
> -    av_dict_set(&dict, NULL, "b", 0);
> -    av_dict_get(dict, NULL, NULL, 0);
> +    if (av_dict_set(&dict, NULL, "a", 0) >= 0 ||
> +        av_dict_set(&dict, NULL, "b", 0) >= 0 ||
> +        av_dict_set(&dict, NULL, NULL, AV_DICT_DONT_STRDUP_KEY) >= 0 ||
> +        av_dict_set(&dict, NULL, av_strdup("b"), AV_DICT_DONT_STRDUP_VAL) >= 0 ||
> +        av_dict_count(dict))
> +        printf("av_dict_set does not correctly handle NULL key\n");
> +
>      e = NULL;
>      while ((e = av_dict_get(dict, "", e, AV_DICT_IGNORE_SUFFIX)))
>          printf("'%s' '%s'\n", e->key, e->value);

Will apply this tonight unless there are objections.

- Andreas