diff mbox series

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

Message ID GV1P250MB0737D119F08A4119C467AFA88F469@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM
State Accepted
Commit 187cd278325ae2fc6d533141e7d6c557b669932a
Headers show
Series [FFmpeg-devel] avutil/dict: Error out in case of key == NULL | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 14, 2022, 7:55 p.m. UTC 
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@outlook.com>
---
 libavutil/dict.c       | 27 +++++++++++++++++----------
 libavutil/tests/dict.c | 12 +++++++++---
 2 files changed, 26 insertions(+), 13 deletions(-)

Comments

Andreas Rheinhardt Sept. 19, 2022, 2:27 p.m. UTC | #1 
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@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
diff mbox series

Patch

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);