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

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