diff mbox series

[v3] make check-unit: use after free in test-opts-visitor

Message ID 1565024586-387112-1-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series [v3] make check-unit: use after free in test-opts-visitor | expand

Commit Message

Andrey Shinkevich Aug. 5, 2019, 5:03 p.m. UTC
In the struct OptsVisitor, the '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 reproduced by running the test case
under the Valgrind: valgrind tests/test-opts-visitor.
A new mode ListMode::LM_TRAVERSED is declared to mark the list
traversal completed.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---

v3:
 01: The comment of the patch header was amended.
 02: The change in spacing of 'ListMode' comment blocks was rolled back.
 03: The 'repeated_opts' in opts_end_list() is now reset unconditionally
     as it was.
 04: The 'name' in the error_setg() was removed as the pointer to the list 
     name can be null (suggested by Markus).

 qapi/opts-visitor.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

Comments

Andrey Shinkevich Aug. 20, 2019, 3:17 p.m. UTC | #1
PINGING...

On 05/08/2019 20:03, Andrey Shinkevich wrote:
> In the struct OptsVisitor, the '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 reproduced by running the test case
> under the Valgrind: valgrind tests/test-opts-visitor.
> A new mode ListMode::LM_TRAVERSED is declared to mark the list
> traversal completed.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
> 
> v3:
>   01: The comment of the patch header was amended.
>   02: The change in spacing of 'ListMode' comment blocks was rolled back.
>   03: The 'repeated_opts' in opts_end_list() is now reset unconditionally
>       as it was.
>   04: The 'name' in the error_setg() was removed as the pointer to the list
>       name can be null (suggested by Markus).
> 
>   qapi/opts-visitor.c | 26 ++++++++++++++++++++++----
>   1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 324b197..5fe0276 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -24,7 +24,8 @@ enum ListMode
>   {
>       LM_NONE,             /* not traversing a list of repeated options */
>   
> -    LM_IN_PROGRESS,      /* opts_next_list() ready to be called.
> +    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
> @@ -36,7 +37,8 @@ enum ListMode
>                             * LM_UNSIGNED_INTERVAL.
>                             */
>   
> -    LM_SIGNED_INTERVAL,  /* opts_next_list() has been called.
> +    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,
> @@ -48,7 +50,14 @@ enum ListMode
>                             * next element of the signed interval.
>                             */
>   
> -    LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned 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,7 +302,8 @@ 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->list_mode == LM_UNSIGNED_INTERVAL ||
> +           ov->list_mode == LM_TRAVERSED);
>       ov->repeated_opts = NULL;
>       ov->list_mode = LM_NONE;
>   }
> @@ -306,6 +320,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, "Fewer list elements than expected");
> +        return NULL;
> +    }
>       assert(ov->list_mode == LM_IN_PROGRESS);
>       return g_queue_peek_head(ov->repeated_opts);
>   }
>
Markus Armbruster Aug. 21, 2019, 11:25 a.m. UTC | #2
Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:

> In the struct OptsVisitor, the '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 reproduced by running the test case
> under the Valgrind: valgrind tests/test-opts-visitor.
> A new mode ListMode::LM_TRAVERSED is declared to mark the list
> traversal completed.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Queued.  Thanks!
Andrey Shinkevich Aug. 21, 2019, 11:55 a.m. UTC | #3
On 21/08/2019 14:25, Markus Armbruster wrote:
> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> 
>> In the struct OptsVisitor, the '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 reproduced by running the test case
>> under the Valgrind: valgrind tests/test-opts-visitor.
>> A new mode ListMode::LM_TRAVERSED is declared to mark the list
>> traversal completed.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> Queued.  Thanks!
> 
Thank you very much Markus!
Andrey
diff mbox series

Patch

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 324b197..5fe0276 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -24,7 +24,8 @@  enum ListMode
 {
     LM_NONE,             /* not traversing a list of repeated options */
 
-    LM_IN_PROGRESS,      /* opts_next_list() ready to be called.
+    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
@@ -36,7 +37,8 @@  enum ListMode
                           * LM_UNSIGNED_INTERVAL.
                           */
 
-    LM_SIGNED_INTERVAL,  /* opts_next_list() has been called.
+    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,
@@ -48,7 +50,14 @@  enum ListMode
                           * next element of the signed interval.
                           */
 
-    LM_UNSIGNED_INTERVAL /* Same as above, only for an unsigned 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,7 +302,8 @@  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->list_mode == LM_UNSIGNED_INTERVAL ||
+           ov->list_mode == LM_TRAVERSED);
     ov->repeated_opts = NULL;
     ov->list_mode = LM_NONE;
 }
@@ -306,6 +320,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, "Fewer list elements than expected");
+        return NULL;
+    }
     assert(ov->list_mode == LM_IN_PROGRESS);
     return g_queue_peek_head(ov->repeated_opts);
 }