qobject_from_jsonv() was unusual in that it took a va_list*, instead
of the more typical va_list; this was so that callers could pass
NULL to avoid % interpolation.  While this works under the hood, it
is awkward for callers, so move the magic into qjson.c rather than
in the public interface, and finally improve the documentation of
qobject_from_jsonf().

test-qobject-input-visitor.c's visitor_input_test_init_internal()
was the only caller passing NULL, fix it to instead use a QObject
created by the various callers, who now use the appropriate form
of qobject_from_json*() according to whether % interpolation is
desired.

Once that is done, all remaining callers to qobject_from_jsonv() are
passing &error_abort; drop this parameter to match the counterpart
qobject_from_jsonf() which assert()s success instead.  Besides, all
current callers that need interpolation live in the testsuite, where
enforcing well-defined input by asserts can help catch typos, and
where we should not be operating on dynamic untrusted arbitrary
input in the first place.

Asserting success has another nice benefit: if we pass more than one
%p, but could return failure, we would have to worry about whether
all arguments in the va_list had consistent refcount treatment (it
should be an all-or-none decision on whether each QObject in the
va_list had its reference count altered - but whichever way we
prefer, it's a lot of overhead to ensure we do it for everything
in the va_list even if we failed halfway through).  But now that we
promise success, we can now easily promise that all %p arguments will
now be cleaned up when freeing the returned object.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/qmp/qjson.h           |  2 +-
 tests/libqtest.c                   | 10 ++------
 qobject/qjson.c                    | 49 +++++++++++++++++++++++++++++++++++---
 tests/test-qobject-input-visitor.c | 18 ++++++++------
 4 files changed, 60 insertions(+), 19 deletions(-)

diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
index 6e84082d5f..9aacb1ccf6 100644
--- a/include/qapi/qmp/qjson.h
+++ b/include/qapi/qmp/qjson.h
@@ -19,7 +19,7 @@

 QObject *qobject_from_json(const char *string, Error **errp);
 QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
-QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
+QObject *qobject_from_jsonv(const char *string, va_list ap)
     GCC_FMT_ATTR(1, 0);

 QString *qobject_to_json(const QObject *obj);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 99a07c246f..cde737ec5a 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -448,7 +448,6 @@ QDict *qtest_qmp_receive(QTestState *s)
  */
 void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
 {
-    va_list ap_copy;
     QObject *qobj;
     int log = getenv("QTEST_LOG") != NULL;
     QString *qstr;
@@ -463,13 +462,8 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
     }
     assert(*fmt);

-    /* Going through qobject ensures we escape strings properly.
-     * This seemingly unnecessary copy is required in case va_list
-     * is an array type.
-     */
-    va_copy(ap_copy, ap);
-    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
-    va_end(ap_copy);
+    /* Going through qobject ensures we escape strings properly. */
+    qobj = qobject_from_jsonv(fmt, ap);
     qstr = qobject_to_json(qobj);

     /*
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 2e0930884e..210c290aa9 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -35,7 +35,8 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
     s->result = json_parser_parse_err(tokens, s->ap, &s->err);
 }

-QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
+static QObject *qobject_from_json_internal(const char *string, va_list *ap,
+                                           Error **errp)
 {
     JSONParsingState state = {};

@@ -50,12 +51,31 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
     return state.result;
 }

+/*
+ * Parses JSON input without interpolation.
+ *
+ * Returns a QObject matching the input on success, or NULL with
+ * an error set if the input is not valid JSON.
+ */
 QObject *qobject_from_json(const char *string, Error **errp)
 {
-    return qobject_from_jsonv(string, NULL, errp);
+    return qobject_from_json_internal(string, NULL, errp);
 }

 /*
+ * Parses JSON input with interpolation of % sequences.
+ *
+ * The set of sequences recognized is compatible with gcc's -Wformat
+ * warnings, although not all printf sequences are valid.  All use of
+ * % must occur outside JSON strings.
+ *
+ * %i - treat corresponding integer value as JSON bool
+ * %[l[l]]d, %PRId64 - treat sized integer value as signed JSON number
+ * %[l[l]]u, %PRIu64 - treat sized integer value as unsigned JSON number
+ * %f - treat double as JSON number (undefined for inf, NaN)
+ * %s - convert char * into JSON string (adds escapes, outer quotes)
+ * %p - embed QObject, transferring the reference to the returned object
+ *
  * IMPORTANT: This function aborts on error, thus it must not
  * be used with untrusted arguments.
  */
@@ -65,13 +85,36 @@ QObject *qobject_from_jsonf(const char *string, ...)
     va_list ap;

     va_start(ap, string);
-    obj = qobject_from_jsonv(string, &ap, &error_abort);
+    obj = qobject_from_json_internal(string, &ap, &error_abort);
     va_end(ap);

     assert(obj != NULL);
     return obj;
 }

+/*
+ * va_list form of qobject_from_jsonf().
+ *
+ * IMPORTANT: This function aborts on error, thus it must not
+ * be used with untrusted arguments.
+ */
+QObject *qobject_from_jsonv(const char *string, va_list ap)
+{
+    QObject *obj;
+    va_list ap_copy;
+
+    /*
+     * This seemingly unnecessary copy is required in case va_list
+     * is an array type.
+     */
+    va_copy(ap_copy, ap);
+    obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
+    va_end(ap_copy);
+
+    assert(obj != NULL);
+    return obj;
+}
+
 typedef struct ToJsonIterState
 {
     int indent;
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index bcf02617dc..a9addd9f98 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -45,13 +45,11 @@ static void visitor_input_teardown(TestInputVisitorData *data,
    function so that the JSON string used by the tests are kept in the test
    functions (and not in main()). */
 static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
-                                                 bool keyval,
-                                                 const char *json_string,
-                                                 va_list *ap)
+                                                 bool keyval, QObject *obj)
 {
     visitor_input_teardown(data, NULL);

-    data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
+    data->obj = obj;
     g_assert(data->obj);

     if (keyval) {
@@ -69,10 +67,12 @@ Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
                                       const char *json_string, ...)
 {
     Visitor *v;
+    QObject *obj;
     va_list ap;

     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, keyval, json_string, &ap);
+    obj = qobject_from_jsonv(json_string, ap);
+    v = visitor_input_test_init_internal(data, keyval, obj);
     va_end(ap);
     return v;
 }
@@ -82,10 +82,12 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
                                  const char *json_string, ...)
 {
     Visitor *v;
+    QObject *obj;
     va_list ap;

     va_start(ap, json_string);
-    v = visitor_input_test_init_internal(data, false, json_string, &ap);
+    obj = qobject_from_jsonv(json_string, ap);
+    v = visitor_input_test_init_internal(data, false, obj);
     va_end(ap);
     return v;
 }
@@ -100,7 +102,9 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
 static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
                                             const char *json_string)
 {
-    return visitor_input_test_init_internal(data, false, json_string, NULL);
+    QObject *obj = qobject_from_json(json_string, &error_abort);
+
+    return visitor_input_test_init_internal(data, false, obj);
 }

 static void test_visitor_in_int(TestInputVisitorData *data,
-- 
2.13.3

Eric Blake <eblake@redhat.com> writes:

> qobject_from_jsonv() was unusual in that it took a va_list*, instead
> of the more typical va_list; this was so that callers could pass
> NULL to avoid % interpolation.  While this works under the hood, it
> is awkward for callers, so move the magic into qjson.c rather than
> in the public interface, and finally improve the documentation of
> qobject_from_jsonf().
>
> test-qobject-input-visitor.c's visitor_input_test_init_internal()
> was the only caller passing NULL, fix it to instead use a QObject
> created by the various callers, who now use the appropriate form
> of qobject_from_json*() according to whether % interpolation is
> desired.
>
> Once that is done, all remaining callers to qobject_from_jsonv() are
> passing &error_abort; drop this parameter to match the counterpart
> qobject_from_jsonf() which assert()s success instead.  Besides, all
> current callers that need interpolation live in the testsuite, where
> enforcing well-defined input by asserts can help catch typos, and
> where we should not be operating on dynamic untrusted arbitrary
> input in the first place.
>
> Asserting success has another nice benefit: if we pass more than one
> %p, but could return failure, we would have to worry about whether
> all arguments in the va_list had consistent refcount treatment (it
> should be an all-or-none decision on whether each QObject in the
> va_list had its reference count altered - but whichever way we
> prefer, it's a lot of overhead to ensure we do it for everything
> in the va_list even if we failed halfway through).  But now that we
> promise success, we can now easily promise that all %p arguments will
> now be cleaned up when freeing the returned object.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/qapi/qmp/qjson.h           |  2 +-
>  tests/libqtest.c                   | 10 ++------
>  qobject/qjson.c                    | 49 +++++++++++++++++++++++++++++++++++---
>  tests/test-qobject-input-visitor.c | 18 ++++++++------
>  4 files changed, 60 insertions(+), 19 deletions(-)
>
> diff --git a/include/qapi/qmp/qjson.h b/include/qapi/qmp/qjson.h
> index 6e84082d5f..9aacb1ccf6 100644
> --- a/include/qapi/qmp/qjson.h
> +++ b/include/qapi/qmp/qjson.h
> @@ -19,7 +19,7 @@
>
>  QObject *qobject_from_json(const char *string, Error **errp);
>  QObject *qobject_from_jsonf(const char *string, ...) GCC_FMT_ATTR(1, 2);
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +QObject *qobject_from_jsonv(const char *string, va_list ap)
>      GCC_FMT_ATTR(1, 0);
>
>  QString *qobject_to_json(const QObject *obj);
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 99a07c246f..cde737ec5a 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -448,7 +448,6 @@ QDict *qtest_qmp_receive(QTestState *s)
>   */
>  void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>  {
> -    va_list ap_copy;
>      QObject *qobj;
>      int log = getenv("QTEST_LOG") != NULL;
>      QString *qstr;
> @@ -463,13 +462,8 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>      }
>      assert(*fmt);
>
> -    /* Going through qobject ensures we escape strings properly.
> -     * This seemingly unnecessary copy is required in case va_list
> -     * is an array type.
> -     */
> -    va_copy(ap_copy, ap);
> -    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
> -    va_end(ap_copy);
> +    /* Going through qobject ensures we escape strings properly. */
> +    qobj = qobject_from_jsonv(fmt, ap);
>      qstr = qobject_to_json(qobj);
>
>      /*

Wait!  Oh, the va_copy() moves iinto qobject_from_jsonv().  Okay, I
guess.

> diff --git a/qobject/qjson.c b/qobject/qjson.c
> index 2e0930884e..210c290aa9 100644
> --- a/qobject/qjson.c
> +++ b/qobject/qjson.c
> @@ -35,7 +35,8 @@ static void parse_json(JSONMessageParser *parser, GQueue *tokens)
>      s->result = json_parser_parse_err(tokens, s->ap, &s->err);
>  }
>
> -QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
> +static QObject *qobject_from_json_internal(const char *string, va_list *ap,
> +                                           Error **errp)
>  {
>      JSONParsingState state = {};
>
> @@ -50,12 +51,31 @@ QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
>      return state.result;
>  }
>
> +/*
> + * Parses JSON input without interpolation.

Imperative mood, please.  Same elsewhere.

Suggest "without interpolation of % sequences".

> + *
> + * Returns a QObject matching the input on success, or NULL with
> + * an error set if the input is not valid JSON.
> + */
>  QObject *qobject_from_json(const char *string, Error **errp)
>  {
> -    return qobject_from_jsonv(string, NULL, errp);
> +    return qobject_from_json_internal(string, NULL, errp);
>  }
>
>  /*
> + * Parses JSON input with interpolation of % sequences.
> + *
> + * The set of sequences recognized is compatible with gcc's -Wformat
> + * warnings, although not all printf sequences are valid.  All use of
> + * % must occur outside JSON strings.
> + *
> + * %i - treat corresponding integer value as JSON bool
> + * %[l[l]]d, %PRId64 - treat sized integer value as signed JSON number
> + * %[l[l]]u, %PRIu64 - treat sized integer value as unsigned JSON number
> + * %f - treat double as JSON number (undefined for inf, NaN)
> + * %s - convert char * into JSON string (adds escapes, outer quotes)
> + * %p - embed QObject, transferring the reference to the returned object
> + *
>   * IMPORTANT: This function aborts on error, thus it must not
>   * be used with untrusted arguments.
>   */

Use the opportunity to stop shouting IMPORTANT?

> @@ -65,13 +85,36 @@ QObject *qobject_from_jsonf(const char *string, ...)
>      va_list ap;
>
>      va_start(ap, string);
> -    obj = qobject_from_jsonv(string, &ap, &error_abort);
> +    obj = qobject_from_json_internal(string, &ap, &error_abort);
>      va_end(ap);
>
>      assert(obj != NULL);
>      return obj;
>  }
>
> +/*
> + * va_list form of qobject_from_jsonf().
> + *
> + * IMPORTANT: This function aborts on error, thus it must not
> + * be used with untrusted arguments.
> + */
> +QObject *qobject_from_jsonv(const char *string, va_list ap)
> +{
> +    QObject *obj;
> +    va_list ap_copy;
> +
> +    /*
> +     * This seemingly unnecessary copy is required in case va_list
> +     * is an array type.
> +     */

--verbose?

> +    va_copy(ap_copy, ap);
> +    obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
> +    va_end(ap_copy);
> +
> +    assert(obj != NULL);
> +    return obj;
> +}
> +
>  typedef struct ToJsonIterState
>  {
>      int indent;
> diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
> index bcf02617dc..a9addd9f98 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -45,13 +45,11 @@ static void visitor_input_teardown(TestInputVisitorData *data,
>     function so that the JSON string used by the tests are kept in the test
>     functions (and not in main()). */
>  static Visitor *visitor_input_test_init_internal(TestInputVisitorData *data,
> -                                                 bool keyval,
> -                                                 const char *json_string,
> -                                                 va_list *ap)
> +                                                 bool keyval, QObject *obj)
>  {
>      visitor_input_teardown(data, NULL);
>
> -    data->obj = qobject_from_jsonv(json_string, ap, &error_abort);
> +    data->obj = obj;
>      g_assert(data->obj);
>
>      if (keyval) {
> @@ -69,10 +67,12 @@ Visitor *visitor_input_test_init_full(TestInputVisitorData *data,
>                                        const char *json_string, ...)
>  {
>      Visitor *v;
> +    QObject *obj;
>      va_list ap;
>
>      va_start(ap, json_string);
> -    v = visitor_input_test_init_internal(data, keyval, json_string, &ap);
> +    obj = qobject_from_jsonv(json_string, ap);
> +    v = visitor_input_test_init_internal(data, keyval, obj);
>      va_end(ap);
>      return v;
>  }
> @@ -82,10 +82,12 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
>                                   const char *json_string, ...)
>  {
>      Visitor *v;
> +    QObject *obj;
>      va_list ap;
>
>      va_start(ap, json_string);
> -    v = visitor_input_test_init_internal(data, false, json_string, &ap);
> +    obj = qobject_from_jsonv(json_string, ap);
> +    v = visitor_input_test_init_internal(data, false, obj);
>      va_end(ap);
>      return v;
>  }
> @@ -100,7 +102,9 @@ Visitor *visitor_input_test_init(TestInputVisitorData *data,
>  static Visitor *visitor_input_test_init_raw(TestInputVisitorData *data,
>                                              const char *json_string)
>  {
> -    return visitor_input_test_init_internal(data, false, json_string, NULL);
> +    QObject *obj = qobject_from_json(json_string, &error_abort);
> +
> +    return visitor_input_test_init_internal(data, false, obj);
>  }
>
>  static void test_visitor_in_int(TestInputVisitorData *data,

On 08/09/2017 02:59 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> qobject_from_jsonv() was unusual in that it took a va_list*, instead
>> of the more typical va_list; this was so that callers could pass
>> NULL to avoid % interpolation.  While this works under the hood, it
>> is awkward for callers, so move the magic into qjson.c rather than
>> in the public interface, and finally improve the documentation of
>> qobject_from_jsonf().
>>

>> -    /* Going through qobject ensures we escape strings properly.
>> -     * This seemingly unnecessary copy is required in case va_list
>> -     * is an array type.
>> -     */
>> -    va_copy(ap_copy, ap);
>> -    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
>> -    va_end(ap_copy);
>> +    /* Going through qobject ensures we escape strings properly. */
>> +    qobj = qobject_from_jsonv(fmt, ap);
>>      qstr = qobject_to_json(qobj);
>>
>>      /*
> 
> Wait!  Oh, the va_copy() moves iinto qobject_from_jsonv().  Okay, I
> guess.


>> +
>> +    /*
>> +     * This seemingly unnecessary copy is required in case va_list
>> +     * is an array type.
>> +     */
> 
> --verbose?

Code motion. But if the comment needs to be more verbose in the
destination than it was on the source, the rationale is that C99/POSIX
allows 'typedef something va_list[]' (that is, where va_list is an array
of some other type), although I don't know of any modern OS that
actually defines it like that.  Based on C pointer-decay rules, '&ap'
has a different type based on whether va_list was a struct/pointer or an
array type, when 'va_list ap' was passed as a parameter; so we can't
portably use qobject_from_json_internal(string, &ap, &error_abort).  The
va_copy() is what lets us guarantee that &ap_list is a pointer to a
va_list regardless of the type of va_list (because va_copy was declared
locally, rather than in a parameter list, and is therefore not subject
to pointer decay), and NOT an accidental pointer to first element of the
va_list array on platforms where va_list is an array.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 08/09/2017 02:59 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> qobject_from_jsonv() was unusual in that it took a va_list*, instead
>>> of the more typical va_list; this was so that callers could pass
>>> NULL to avoid % interpolation.  While this works under the hood, it
>>> is awkward for callers, so move the magic into qjson.c rather than
>>> in the public interface, and finally improve the documentation of
>>> qobject_from_jsonf().
>>>
>
>>> -    /* Going through qobject ensures we escape strings properly.
>>> -     * This seemingly unnecessary copy is required in case va_list
>>> -     * is an array type.
>>> -     */
>>> -    va_copy(ap_copy, ap);
>>> -    qobj = qobject_from_jsonv(fmt, &ap_copy, &error_abort);
>>> -    va_end(ap_copy);
>>> +    /* Going through qobject ensures we escape strings properly. */
>>> +    qobj = qobject_from_jsonv(fmt, ap);
>>>      qstr = qobject_to_json(qobj);
>>>
>>>      /*
>> 
>> Wait!  Oh, the va_copy() moves iinto qobject_from_jsonv().  Okay, I
>> guess.
[...]
>>> +
>>> +    /*
>>> +     * This seemingly unnecessary copy is required in case va_list
>>> +     * is an array type.
>>> +     */
>> 
>> --verbose?
>>
>>> +    va_copy(ap_copy, ap);
>>> +    obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
>>> +    va_end(ap_copy);
>
> Code motion. But if the comment needs to be more verbose in the
> destination than it was on the source, the rationale is that C99/POSIX
> allows 'typedef something va_list[]' (that is, where va_list is an array
> of some other type), although I don't know of any modern OS that
> actually defines it like that.  Based on C pointer-decay rules, '&ap'
> has a different type based on whether va_list was a struct/pointer or an
> array type, when 'va_list ap' was passed as a parameter; so we can't
> portably use qobject_from_json_internal(string, &ap, &error_abort).  The
> va_copy() is what lets us guarantee that &ap_list is a pointer to a
> va_list regardless of the type of va_list (because va_copy was declared
> locally, rather than in a parameter list, and is therefore not subject
> to pointer decay), and NOT an accidental pointer to first element of the
> va_list array on platforms where va_list is an array.

I'm dense this Monday morning --- I still can't see where exactly
passing &ap directly goes wrong.

Two cases:

1. va_list is a typedef name for a non-array type T.

2. va_list is a typedef name for an array type E[].

What are the types of actual argument &ap and formal parameter va_list
*ap in either case?

How exactly does case 2 break?

On 10/02/2017 12:46 AM, Markus Armbruster wrote:

>>>> +    /*
>>>> +     * This seemingly unnecessary copy is required in case va_list
>>>> +     * is an array type.
>>>> +     */
>>>
>>> --verbose?
>>>
>>>> +    va_copy(ap_copy, ap);
>>>> +    obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
>>>> +    va_end(ap_copy);
>>
>> Code motion. But if the comment needs to be more verbose in the
>> destination than it was on the source, the rationale is that C99/POSIX
>> allows 'typedef something va_list[]' (that is, where va_list is an array
>> of some other type), although I don't know of any modern OS that
>> actually defines it like that.  Based on C pointer-decay rules, '&ap'
>> has a different type based on whether va_list was a struct/pointer or an
>> array type, when 'va_list ap' was passed as a parameter; so we can't
>> portably use qobject_from_json_internal(string, &ap, &error_abort).  The
>> va_copy() is what lets us guarantee that &ap_list is a pointer to a
>> va_list regardless of the type of va_list (because va_copy was declared
>> locally, rather than in a parameter list, and is therefore not subject
>> to pointer decay), and NOT an accidental pointer to first element of the
>> va_list array on platforms where va_list is an array.
> 
> I'm dense this Monday morning --- I still can't see where exactly
> passing &ap directly goes wrong.
> 
> Two cases:
> 
> 1. va_list is a typedef name for a non-array type T.
> 
> 2. va_list is a typedef name for an array type E[].
> 
> What are the types of actual argument &ap and formal parameter va_list
> *ap in either case?
> 
> How exactly does case 2 break?

An example program is probably the best to visualize the problem:

$ cat typefun.c
#include <stdio.h>

typedef struct T {
    int i[5];
} T;
typedef struct E {
    int i;
} E;

typedef T list1;
typedef E list2[5];

void bar(const char *prefix, list1 *l1, list2 *l2) {
    printf ("%s: %zu %zu\n", prefix, sizeof(&l1), sizeof(&l2));
}

void foo(list1 l1, list2 l2) {
    printf ("parameter sizes: %zu %zu\n", sizeof(l1), sizeof(l2));
    bar("called with address of parameter", &l1, &l2);
}

int main(void) {
    list1 l1;
    list2 l2;
    printf ("local variable sizes: %zu %zu\n", sizeof(l1), sizeof(l2));
    bar("called with address of local variable", &l1, &l2);
    foo(l1, l2);
    return 0;
}

$ gcc -o typefun -Wall typefun.c
typefun.c: In function ‘foo’:
typefun.c:18:61: warning: ‘sizeof’ on array function parameter ‘l2’ will
return size of ‘E * {aka struct E *}’ [-Wsizeof-array-argument]
     printf ("parameter sizes: %zu %zu\n", sizeof(l1), sizeof(l2));
                                                             ^
typefun.c:17:26: note: declared here
 void foo(list1 l1, list2 l2) {
                          ^~
typefun.c:19:50: warning: passing argument 3 of ‘bar’ from incompatible
pointer type [-Wincompatible-pointer-types]
     bar("called with address of parameter", &l1, &l2);
                                                  ^
typefun.c:13:6: note: expected ‘E (*)[5] {aka struct E (*)[5]}’ but
argument is of type ‘E ** {aka struct E **}’
 void bar(const char *prefix, list1 *l1, list2 *l2) {
      ^~~

$ ./typefun
local variable sizes: 20 20
called with address of local variable: 8 8
parameter sizes: 20 8
called with address of parameter: 8 8

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 10/02/2017 09:30 AM, Eric Blake wrote:
> On 10/02/2017 12:46 AM, Markus Armbruster wrote:
> 
>>>>> +    /*
>>>>> +     * This seemingly unnecessary copy is required in case va_list
>>>>> +     * is an array type.
>>>>> +     */
>>>>
>>>> --verbose?
>>>>
>>>>> +    va_copy(ap_copy, ap);
>>>>> +    obj = qobject_from_json_internal(string, &ap_copy, &error_abort);
>>>>> +    va_end(ap_copy);
>>>
>>> Code motion. But if the comment needs to be more verbose in the
>>> destination than it was on the source, the rationale is that C99/POSIX
>>> allows 'typedef something va_list[]' (that is, where va_list is an array
>>> of some other type), although I don't know of any modern OS that
>>> actually defines it like that.  Based on C pointer-decay rules, '&ap'
>>> has a different type based on whether va_list was a struct/pointer or an
>>> array type, when 'va_list ap' was passed as a parameter; so we can't
>>> portably use qobject_from_json_internal(string, &ap, &error_abort).  The
>>> va_copy() is what lets us guarantee that &ap_list is a pointer to a
>>> va_list regardless of the type of va_list (because va_copy was declared
>>> locally, rather than in a parameter list, and is therefore not subject
>>> to pointer decay), and NOT an accidental pointer to first element of the
>>> va_list array on platforms where va_list is an array.

When I first wrote that, I didn't know where it mattered in practice;
now I do.  Here's a gcc non-bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14557

that documents a workaround of using a macro va_ptr() because there ARE
existing modern platforms that use an array type for va_list:
https://sourceware.org/ml/newlib/2017/msg01302.html

I don't know if va_ptr() is any more elegant than the va_copy() used in
my code motion.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 08/03/2017 08:25 PM, Eric Blake wrote:
> qobject_from_jsonv() was unusual in that it took a va_list*, instead
> of the more typical va_list; this was so that callers could pass
> NULL to avoid % interpolation.  While this works under the hood, it
> is awkward for callers, so move the magic into qjson.c rather than
> in the public interface, and finally improve the documentation of
> qobject_from_jsonf().
> 

> +/*
> + * va_list form of qobject_from_jsonf().
> + *
> + * IMPORTANT: This function aborts on error, thus it must not
> + * be used with untrusted arguments.
> + */
> +QObject *qobject_from_jsonv(const char *string, va_list ap)
> +{

Given your comments on vararg naming elsewhere in the series, I'm also
thinking this is a good chance to fix this to be named
qobject_from_vjsonf() (making it obvious it is the va_list form of
formatted json, and similar to printf/vprintf or my
qtest_startf/qtest_vstartf in my v7 preliminary cleanup series).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@redhat.com> writes:

> On 08/03/2017 08:25 PM, Eric Blake wrote:
>> qobject_from_jsonv() was unusual in that it took a va_list*, instead
>> of the more typical va_list; this was so that callers could pass
>> NULL to avoid % interpolation.  While this works under the hood, it
>> is awkward for callers, so move the magic into qjson.c rather than
>> in the public interface, and finally improve the documentation of
>> qobject_from_jsonf().
>> 
>
>> +/*
>> + * va_list form of qobject_from_jsonf().
>> + *
>> + * IMPORTANT: This function aborts on error, thus it must not
>> + * be used with untrusted arguments.
>> + */
>> +QObject *qobject_from_jsonv(const char *string, va_list ap)
>> +{
>
> Given your comments on vararg naming elsewhere in the series, I'm also
> thinking this is a good chance to fix this to be named
> qobject_from_vjsonf() (making it obvious it is the va_list form of
> formatted json, and similar to printf/vprintf or my
> qtest_startf/qtest_vstartf in my v7 preliminary cleanup series).

Yes, please.