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

[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 <armbru@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---

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

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

[Markus Armbruster]

Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:

> 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.

Let's mention the reproducer: valgrind tests/test/opts-visitor.

>                                  Also, the Visitor object call back
> functions are supposed to set the Error parameter in case of failure.

As far as I can tell, they all do.  The only place where you set an
error is the new failure you add to lookup_scalar().

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

Please don't change the spacing without need.  The hunk should look like
this:

  @@ -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,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;
> +    }

What's wrong with zapping ov->repeated_opts unconditionally?

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

Beware, @name is null when visiting list members.  The test still passes
for me, since g_strdup_vprintf() formats a null argument to %s as
"(null)".

For what it's worth, the qobject input visitor uses
QERR_MISSING_PARAMETER with a made-up name.  Computing the name is
pretty elaborate, see full_name_nth().  I'd rather not duplicate that
here.

Suggest something like

           error_setg(errp, "Fewer list elements than expected");

The error message fails to mention the name of the list.  Bad, but the
error is a corner case; we don't normally visit beyond the end of the
list.  For a better message, we'd have to have start_list() store its
@name in struct OptsVisitor.  I'm not asking you to do that now.

> +        return NULL;
> +    }
>      assert(ov->list_mode == LM_IN_PROGRESS);
>      return g_queue_peek_head(ov->repeated_opts);
>  }

I checked the remaining uses of ->list_mode, and I think they are okay.

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

[Andrey Shinkevich]

On 02/08/2019 14:34, Markus Armbruster wrote:
> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
> 
>> 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.
> 
> Let's mention the reproducer: valgrind tests/test/opts-visitor.
> 
>>                                   Also, the Visitor object call back
>> functions are supposed to set the Error parameter in case of failure.
> 
> As far as I can tell, they all do.  The only place where you set an
> error is the new failure you add to lookup_scalar().
> 

The story behind the comment is that the original 
tests/test-opts-visitor fails being run under the Valgrind with the 
error message:

test-opts-visitor: util/error.c:276: error_free_or_abort: Assertion 
`errp && *errp' failed.

coming from

assert(errp && *errp);
error_free_or_abort (util/error.c:276)
test_opts_range_beyond (tests/test-opts-visitor.c:241)

because g_queue_peek_head() returns NULL under the Valgrind and errp 
stays unset.

Without the Valgrind, the g_queue_peek_head() returns a non-zero pointer 
and the opts_type_int64() sets the following error:

"Parameter '\340F\212\274\267U' expects an int64 value or range", 
err_class = ERROR_CLASS_GENERIC_ERROR, src = 0x55b7bbc02163 
"qapi/opts-visitor.c",
   func = 0x55b7bbc02410 <__func__.14916> "opts_type_int64", line = 433, 
hint = 0x0}

so, error_free_or_abort() doesn't abort and the test case passes.

I will remove the comment in v3.

Andrey

>> 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>
>> ---
>>
>> 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;
> 
> Please don't change the spacing without need.  The hunk should look like
> this:
> 
>    @@ -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,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;
>> +    }
> 
> What's wrong with zapping ov->repeated_opts unconditionally?
> 
>>       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);
> 
> Beware, @name is null when visiting list members.  The test still passes
> for me, since g_strdup_vprintf() formats a null argument to %s as
> "(null)".
> 
> For what it's worth, the qobject input visitor uses
> QERR_MISSING_PARAMETER with a made-up name.  Computing the name is
> pretty elaborate, see full_name_nth().  I'd rather not duplicate that
> here.
> 
> Suggest something like
> 
>             error_setg(errp, "Fewer list elements than expected");
> 
> The error message fails to mention the name of the list.  Bad, but the
> error is a corner case; we don't normally visit beyond the end of the
> list.  For a better message, we'd have to have start_list() store its
> @name in struct OptsVisitor.  I'm not asking you to do that now.
> 

Will I do that work to add the @name of the list to the struct 
OptsVisitor in a following series?

>> +        return NULL;
>> +    }
>>       assert(ov->list_mode == LM_IN_PROGRESS);
>>       return g_queue_peek_head(ov->repeated_opts);
>>   }
> 
> I checked the remaining uses of ->list_mode, and I think they are okay.
> 
Thank you. I also had not noticed any potential issue with the list_mode.

Andrey
-- 
With the best regards,
Andrey Shinkevich

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

[Markus Armbruster]

Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:

> On 02/08/2019 14:34, Markus Armbruster wrote:
>> Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> writes:
>> 
>>> 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.
>> 
>> Let's mention the reproducer: valgrind tests/test/opts-visitor.
>> 
>>>                                   Also, the Visitor object call back
>>> functions are supposed to set the Error parameter in case of failure.
>> 
>> As far as I can tell, they all do.  The only place where you set an
>> error is the new failure you add to lookup_scalar().
>> 
>
> The story behind the comment is that the original 
> tests/test-opts-visitor fails being run under the Valgrind with the 
> error message:
>
> test-opts-visitor: util/error.c:276: error_free_or_abort: Assertion 
> `errp && *errp' failed.
>
> coming from
>
> assert(errp && *errp);
> error_free_or_abort (util/error.c:276)
> test_opts_range_beyond (tests/test-opts-visitor.c:241)
>
> because g_queue_peek_head() returns NULL under the Valgrind and errp 
> stays unset.
>
> Without the Valgrind, the g_queue_peek_head() returns a non-zero pointer 
> and the opts_type_int64() sets the following error:
>
> "Parameter '\340F\212\274\267U' expects an int64 value or range", 
> err_class = ERROR_CLASS_GENERIC_ERROR, src = 0x55b7bbc02163 
> "qapi/opts-visitor.c",
>    func = 0x55b7bbc02410 <__func__.14916> "opts_type_int64", line = 433, 
> hint = 0x0}
>
> so, error_free_or_abort() doesn't abort and the test case passes.
>
> I will remove the comment in v3.

Thanks.

[...]
>>> 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
[...]
>>> @@ -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;
>>> +    }
>> 
>> What's wrong with zapping ov->repeated_opts unconditionally?
>> 
>>>       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);
>> 
>> Beware, @name is null when visiting list members.  The test still passes
>> for me, since g_strdup_vprintf() formats a null argument to %s as
>> "(null)".
>> 
>> For what it's worth, the qobject input visitor uses
>> QERR_MISSING_PARAMETER with a made-up name.  Computing the name is
>> pretty elaborate, see full_name_nth().  I'd rather not duplicate that
>> here.
>> 
>> Suggest something like
>> 
>>             error_setg(errp, "Fewer list elements than expected");
>> 
>> The error message fails to mention the name of the list.  Bad, but the
>> error is a corner case; we don't normally visit beyond the end of the
>> list.  For a better message, we'd have to have start_list() store its
>> @name in struct OptsVisitor.  I'm not asking you to do that now.
>> 
>
> Will I do that work to add the @name of the list to the struct 
> OptsVisitor in a following series?

Entirely up to you.  To be honest, I'm not sure it's worth the trouble.

>>> +        return NULL;
>>> +    }
>>>       assert(ov->list_mode == LM_IN_PROGRESS);
>>>       return g_queue_peek_head(ov->repeated_opts);
>>>   }
>> 
>> I checked the remaining uses of ->list_mode, and I think they are okay.
>> 
> Thank you. I also had not noticed any potential issue with the list_mode.

Good :)