[Qemu-devel] [PATCH v2] make check-unit: use after free in test-opts-visitor

[Andrey Shinkevich]

In struct OptsVisitor, repeated_opts member points to a list in the
unprocessed_opts hash table after the list has been destroyed. A
subsequent call to visit_type_int() references the deleted list. It
results in use-after-free issue. Also, the Visitor object call back
functions are supposed to set the Error parameter in case of failure.
A new mode ListMode::LM_TRAVERSED is declared to mark the list
traversal completed.

Suggested-by: Markus Armbruster <address@hidden>
Signed-off-by: Andrey Shinkevich <address@hidden>
---

v2:
 01: A new mode LM_TRAVERSED has been introduced to check instead of the
     repeated_opts pointer for NULL.

 qapi/opts-visitor.c | 78 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 29 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 324b197..23ac383 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -22,33 +22,42 @@
 
 enum ListMode
 {
-    LM_NONE,             /* not traversing a list of repeated options */
-
-    LM_IN_PROGRESS,      /* opts_next_list() ready to be called.
-                          *
-                          * Generating the next list link will consume the most
-                          * recently parsed QemuOpt instance of the repeated
-                          * option.
-                          *
-                          * Parsing a value into the list link will examine the
-                          * next QemuOpt instance of the repeated option, and
-                          * possibly enter LM_SIGNED_INTERVAL or
-                          * LM_UNSIGNED_INTERVAL.
-                          */
-
-    LM_SIGNED_INTERVAL,  /* opts_next_list() has been called.
-                          *
-                          * Generating the next list link will consume the most
-                          * recently stored element from the signed interval,
-                          * parsed from the most recent QemuOpt instance of the
-                          * repeated option. This may consume QemuOpt itself
-                          * and return to LM_IN_PROGRESS.
-                          *
-                          * Parsing a value into the list link will store the
-                          * next element of the signed interval.
-                          */
-
-    LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned interval. */
+    LM_NONE,              /* not traversing a list of repeated options */
+
+    LM_IN_PROGRESS,       /*
+                           * opts_next_list() ready to be called.
+                           *
+                           * Generating the next list link will consume the 
most
+                           * recently parsed QemuOpt instance of the repeated
+                           * option.
+                           *
+                           * Parsing a value into the list link will examine 
the
+                           * next QemuOpt instance of the repeated option, and
+                           * possibly enter LM_SIGNED_INTERVAL or
+                           * LM_UNSIGNED_INTERVAL.
+                           */
+
+    LM_SIGNED_INTERVAL,   /*
+                           * opts_next_list() has been called.
+                           *
+                           * Generating the next list link will consume the 
most
+                           * recently stored element from the signed interval,
+                           * parsed from the most recent QemuOpt instance of 
the
+                           * repeated option. This may consume QemuOpt itself
+                           * and return to LM_IN_PROGRESS.
+                           *
+                           * Parsing a value into the list link will store the
+                           * next element of the signed interval.
+                           */
+
+    LM_UNSIGNED_INTERVAL, /* Same as above, only for an unsigned interval. */
+
+    LM_TRAVERSED          /*
+                           * opts_next_list() has been called.
+                           *
+                           * No more QemuOpt instance in the list.
+                           * The traversal has been completed.
+                           */
 };
 
 typedef enum ListMode ListMode;
@@ -238,6 +247,8 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
     OptsVisitor *ov = to_ov(v);
 
     switch (ov->list_mode) {
+    case LM_TRAVERSED:
+        return NULL;
     case LM_SIGNED_INTERVAL:
     case LM_UNSIGNED_INTERVAL:
         if (ov->list_mode == LM_SIGNED_INTERVAL) {
@@ -258,6 +269,8 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
         opt = g_queue_pop_head(ov->repeated_opts);
         if (g_queue_is_empty(ov->repeated_opts)) {
             g_hash_table_remove(ov->unprocessed_opts, opt->name);
+            ov->repeated_opts = NULL;
+            ov->list_mode = LM_TRAVERSED;
             return NULL;
         }
         break;
@@ -289,8 +302,11 @@ opts_end_list(Visitor *v, void **obj)
 
     assert(ov->list_mode == LM_IN_PROGRESS ||
            ov->list_mode == LM_SIGNED_INTERVAL ||
-           ov->list_mode == LM_UNSIGNED_INTERVAL);
-    ov->repeated_opts = NULL;
+           ov->list_mode == LM_UNSIGNED_INTERVAL ||
+           ov->list_mode == LM_TRAVERSED);
+    if (ov->list_mode != LM_TRAVERSED) {
+        ov->repeated_opts = NULL;
+    }
     ov->list_mode = LM_NONE;
 }
 
@@ -306,6 +322,10 @@ lookup_scalar(const OptsVisitor *ov, const char *name, 
Error **errp)
         list = lookup_distinct(ov, name, errp);
         return list ? g_queue_peek_tail(list) : NULL;
     }
+    if (ov->list_mode == LM_TRAVERSED) {
+        error_setg(errp, QERR_INVALID_PARAMETER, name);
+        return NULL;
+    }
     assert(ov->list_mode == LM_IN_PROGRESS);
     return g_queue_peek_head(ov->repeated_opts);
 }
-- 
1.8.3.1