Re: [Qemu-devel] [PATCH v14 19/19] qapi: Change visit_type_FOO() to no longer return partial objects

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Returning a partial object on error is an invitation for a careless
> caller to leak memory.  As no one outside the testsuite was actually
> relying on these semantics, it is cleaner to just document and
> guarantee that ALL pointer-based visit_type_FOO() functions always
> leave a safe value in *obj during an input visitor (either the new
> object on success, or NULL if an error is encountered), so callers
> can now unconditionally use qapi_free_FOO() to clean up regardless
> of whether an error occurred.

Hmm, wasn't the "assign null on error" part done in a prior patch?
Checking...  no, only half of it, in PATCH 03: there, we went from "may
store an incomplete object on error" to "store either an incomplete
object or null on error".  Now we go on to just "store null on error".
Correct?

> The decision is done by enhancing qapi-visit-core to return true
> for input visitors (the callbacks themselves do not need
> modification); since we've documented that visit_end_* must be
> called after any successful visit_start_*, that is a sufficient
> point for knowing that something was allocated during start.

I find this sentence a bit confusing.  Let me try:

    To help visitor-agnostic code, such as the generated qapi-visit.c,
    make the visit_end_FOO() return true when something was allocated.
    Easily done in the visitor core, no need to change the callbacks.

But see my comments on the visit_end_FOO() inline.

> Note that we still leave *obj unchanged after a scalar-based
> visit_type_FOO(); I did not feel like auditing all uses of
> visit_type_Enum() to see if the callers would tolerate a specific
> sentinel value (not to mention having to decide whether it would
> be better to use 0 or ENUM__MAX as that sentinel).

Should this note be in PATCH 03?

The inconsistency isn't pretty, but tolerable if it simplifies things.

> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v14: no change
> v13: rebase to latest, documentation tweaks
> v12: rebase to latest, don't modify callback signatures, use newer
> approach for detecting input visitors, avoid 'allocated' boolean
> [no v10, v11]
> v9: fix bug in use of errp
> v8: rebase to earlier changes
> v7: rebase to earlier changes, enhance commit message, also fix
> visit_type_str() and visit_type_any()
> v6: rebase on top of earlier doc and formatting improvements, mention
> that *obj can be uninitialized on entry to an input visitor, rework
> semantics to keep valgrind happy on uninitialized input, break some
> long lines
> ---
>  include/qapi/visitor.h         | 42 ++++++++++++++++++++++++++++++------------
>  include/qapi/visitor-impl.h    |  8 +++++---
>  scripts/qapi-visit.py          | 25 +++++++++++++------------
>  qapi/qapi-visit-core.c         | 41 ++++++++++++++++++++++++++++++++++-------
>  tests/test-qmp-commands.c      | 13 ++++++-------
>  tests/test-qmp-input-strict.c  | 19 ++++++++-----------
>  tests/test-qmp-input-visitor.c | 10 ++--------
>  docs/qapi-code-gen.txt         | 10 ++++++++--
>  8 files changed, 106 insertions(+), 62 deletions(-)
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index a22a7ce..4cc6d3a 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -65,14 +65,16 @@
>   * }' if an error is encountered on "value" (or to have the visitor
>   * core auto-generate the nicer name).
>   *
> - * FIXME: At present, input visitors may allocate an incomplete *@obj
> - * even when visit_type_FOO() reports an error.  Using an output
> - * visitor with an incomplete object has undefined behavior; callers
> - * must call qapi_free_FOO() (which uses the dealloc visitor, and
> - * safely handles an incomplete object) to avoid a memory leak.
> + * If an error is detected during visit_type_FOO() with an input
> + * visitor, then *@obj will be NULL for pointer types, and left
> + * unchanged for scalar types.

Okay.

> + *                              Using an output visitor with an
> + * incomplete object has undefined behavior (other than a special case
> + * for visit_type_str() treating NULL like ""), while the dealloc
> + * visitor safely handles incomplete objects.

Where do the incomplete objects come from now?  I thought this patch
gets rid of them.

>   *
> - * Likewise, the QAPI object types (structs, unions, and alternates)
> - * have a generated function in qapi-visit.h compatible with:
> + * Additionally, the QAPI object types (structs, unions, and
> + * alternates) have a generated function in qapi-visit.h compatible
> + * with:
>   *
>   * void visit_type_FOO_members(Visitor *v, FOO *obj, Error **errp);
>   *

Replaces "Likewise" by "Additionally".  Why not write it this way from
the start?

> @@ -104,7 +106,6 @@
>   *  v = ...obtain input visitor...
>   *  visit_type_Foo(v, NULL, &f, &err);
>   *  if (err) {
> - *      qapi_free_Foo(f);
>   *      ...handle error...
>   *  } else {
>   *      ...use f...
> @@ -112,7 +113,6 @@
>   *  v = ...obtain input visitor...
>   *  visit_type_FooList(v, NULL, &l, &err);
>   *  if (err) {
> - *      qapi_free_FooList(l);
>   *      ...handle error...
>   *  } else {
>   *      while (l) {
> @@ -256,8 +256,14 @@ void visit_check_struct(Visitor *v, Error **errp);
>   * even if intermediate processing was skipped due to errors, to allow
>   * the backend to release any resources.  Destroying the visitor may
>   * behave as if this was implicitly called.
> + *
> + * Returns true if this is an input visitor (that is, an allocation
> + * occurred during visit_start_struct() if obj was non-NULL).  The
> + * caller can use this, along with tracking whether a local error
> + * occurred in the meantime, to decide when to undo allocation before
> + * returning control from a visit_type_FOO() function.
>   */
> -void visit_end_struct(Visitor *v);
> +bool visit_end_struct(Visitor *v);

I generally like functions to return something useful, but not in this
case, because the function name gives you no clue about its value.
Consider:

    if (visit_end_struct(v) && err) {
        qapi_free_FOO(*obj);
        *obj = NULL;
    }

To find out what this means, a reader not familiar with visitors almost
certainly needs to refer to visit_end_struct()'s contract or code.

Compare:

    visit_end_struct(v);
    if (err && v->type == VISITOR_INPUT) {
        qapi_free_FOO(*obj);
        *obj = NULL;
    }

Or:

    visit_end_struct(v);
    if (err && visit_is_input(v)) {
        qapi_free_FOO(*obj);
        *obj = NULL;
    }

>
>
>  /* === Visiting lists */
> @@ -313,8 +319,14 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
>   * even if intermediate processing was skipped due to errors, to allow
>   * the backend to release any resources.  Destroying the visitor may
>   * behave as if this was implicitly called.
> + *
> + * Returns true if this is an input visitor (that is, an allocation
> + * occurred during visit_start_list() if list was non-NULL).  The
> + * caller can use this, along with tracking whether a local error
> + * occurred in the meantime, to decide when to undo allocation before
> + * returning control from a visit_type_FOO() function.
>   */
> -void visit_end_list(Visitor *v);
> +bool visit_end_list(Visitor *v);
>
>
>  /* === Visiting alternates */
> @@ -347,10 +359,16 @@ void visit_start_alternate(Visitor *v, const char *name,
>   * the backend to release any resources.  Destroying the visitor may
>   * behave as if this was implicitly called.
>   *
> + * Returns true if this is an input visitor (that is, an allocation
> + * occurred during visit_start_alternate() if obj was non-NULL).  The
> + * caller can use this, along with tracking whether a local error
> + * occurred in the meantime, to decide when to undo allocation before
> + * returning control from a visit_type_FOO() function.
> + *
>   * TODO: Should all the visit_end_* interfaces take obj parameter, so
>   * that dealloc visitor need not track what was passed in visit_start?
>   */
> -void visit_end_alternate(Visitor *v);
> +bool visit_end_alternate(Visitor *v);
>
>
>  /* === Other helpers */
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 0471465..f113869 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -42,7 +42,8 @@ struct Visitor
>      /* Optional; intended for input visitors.  */
>      void (*check_struct)(Visitor *v, Error **errp);
>
> -    /* Must be set to visit structs.  */
> +    /* Must be set to visit structs.  The core takes care of the
> +     * return value.  */
>      void (*end_struct)(Visitor *v);
>
>      /* Must be set; document if @list may not be NULL.  */
> @@ -52,7 +53,7 @@ struct Visitor
>      /* Must be set.  */
>      GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
>
> -    /* Must be set.  */
> +    /* Must be set.  The core takes care of the return value.  */
>      void (*end_list)(Visitor *v);
>
>      /* Must be set by input and dealloc visitors to visit alternates;
> @@ -61,7 +62,8 @@ struct Visitor
>                              GenericAlternate **obj, size_t size,
>                              bool promote_int, Error **errp);
>
> -    /* Optional, needed for dealloc visitor.  */
> +    /* Optional, needed for dealloc visitor.  The core takes care of
> +     * the return value.  */
>      void (*end_alternate)(Visitor *v);
>
>      /* Must be set.  */
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 69f0133..8f51d8c 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -108,10 +108,6 @@ out:
>
>
>  def gen_visit_list(name, element_type):
> -    # FIXME: if *obj is NULL on entry, and the first visit_next_list()
> -    # assigns to *obj, while a later one fails, we should clean up *obj
> -    # rather than leaving it non-NULL. As currently written, the caller must
> -    # call qapi_free_FOOList() to avoid a memory leak of the partial FOOList.
>      return mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> @@ -131,7 +127,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>              break;
>          }
>      }
> -    visit_end_list(v);
> +    if (visit_end_list(v) && err) {
> +        qapi_free_%(c_name)s(*obj);
> +        *obj = NULL;
> +    }
>  out:
>      error_propagate(errp, err);
>  }
> @@ -208,21 +207,20 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>          error_setg(&err, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "%(name)s");
>      }
> -    visit_end_alternate(v);
> +    if (visit_end_alternate(v) && err) {
> +        qapi_free_%(c_name)s(*obj);
> +        *obj = NULL;
> +    }
>  out:
>      error_propagate(errp, err);
>  }
>  ''',
> -                 name=name)
> +                 name=name, c_name=c_name(name))
>
>      return ret
>
>
>  def gen_visit_object(name, base, members, variants):
> -    # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> -    # *obj, but then visit_type_FOO_members() fails, we should clean up *obj
> -    # rather than leaving it non-NULL. As currently written, the caller must
> -    # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
>      return mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp)
> @@ -242,7 +240,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error
>      }
>      visit_check_struct(v, &err);
>  out_obj:
> -    visit_end_struct(v);
> +    if (visit_end_struct(v) && err) {
> +        qapi_free_%(c_name)s(*obj);
> +        *obj = NULL;
> +    }
>  out:
>      error_propagate(errp, err);
>  }
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 622b315..8477cc0 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -23,11 +23,17 @@
>  void visit_start_struct(Visitor *v, const char *name, void **obj,
>                          size_t size, Error **errp)
>  {
> +    Error *err = NULL;
> +
>      if (obj) {
>          assert(size);
>          assert(v->type != VISITOR_OUTPUT || *obj);
>      }
> -    v->start_struct(v, name, obj, size, errp);
> +    v->start_struct(v, name, obj, size, &err);
> +    if (obj && v->type == VISITOR_INPUT) {
> +        assert(err || *obj);
> +    }
> +    error_propagate(errp, err);

Sure this belongs to this patch?  More of the same below.

>  }
>
>  void visit_check_struct(Visitor *v, Error **errp)
> @@ -37,11 +43,13 @@ void visit_check_struct(Visitor *v, Error **errp)
>      }
>  }
>
> -void visit_end_struct(Visitor *v)
> +bool visit_end_struct(Visitor *v)
>  {
>      v->end_struct(v);
> +    return v->type == VISITOR_INPUT;
>  }
>
> +
>  void visit_start_list(Visitor *v, const char *name, GenericList **list,
>                        size_t size, Error **errp)
>  {
> @@ -55,26 +63,34 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
>      return v->next_list(v, tail, size);
>  }
>
> -void visit_end_list(Visitor *v)
> +bool visit_end_list(Visitor *v)
>  {
>      v->end_list(v);
> +    return v->type == VISITOR_INPUT;
>  }
>
>  void visit_start_alternate(Visitor *v, const char *name,
>                             GenericAlternate **obj, size_t size,
>                             bool promote_int, Error **errp)
>  {
> +    Error *err = NULL;
> +
>      assert(obj && size >= sizeof(GenericAlternate));
>      if (v->start_alternate) {
> -        v->start_alternate(v, name, obj, size, promote_int, errp);
> +        v->start_alternate(v, name, obj, size, promote_int, &err);
> +        if (v->type == VISITOR_INPUT) {
> +            assert(err || *obj);
> +        }
> +        error_propagate(errp, err);
>      }
>  }
>
> -void visit_end_alternate(Visitor *v)
> +bool visit_end_alternate(Visitor *v)
>  {
>      if (v->end_alternate) {
>          v->end_alternate(v);
>      }
> +    return v->type == VISITOR_INPUT;
>  }
>
>  bool visit_optional(Visitor *v, const char *name, bool *present)
> @@ -206,12 +222,17 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
>
>  void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
>  {
> +    Error *err = NULL;
>      assert(obj);
>      /* TODO: Fix callers to not pass NULL when they mean "", so that we
>       * can enable:
>      assert(v->type != VISITOR_OUTPUT || *obj);
>       */
> -    v->type_str(v, name, obj, errp);
> +    v->type_str(v, name, obj, &err);
> +    if (v->type == VISITOR_INPUT) {
> +        assert(err || *obj);
> +    }
> +    error_propagate(errp, err);
>  }
>
>  void visit_type_number(Visitor *v, const char *name, double *obj,
> @@ -223,9 +244,15 @@ void visit_type_number(Visitor *v, const char *name, double *obj,
>
>  void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
>  {
> +    Error *err = NULL;
> +
>      assert(obj);
>      assert(v->type != VISITOR_OUTPUT || *obj);
> -    v->type_any(v, name, obj, errp);
> +    v->type_any(v, name, obj, &err);
> +    if (v->type == VISITOR_INPUT) {
> +        assert(err || *obj);
> +    }
> +    error_propagate(errp, err);
>  }
>
>  void visit_type_null(Visitor *v, const char *name, Error **errp)
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 14a9ebb..d6c494d 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -228,14 +228,13 @@ static void test_dealloc_partial(void)
>          QDECREF(ud2_dict);
>      }
>
> -    /* verify partial success */
> -    assert(ud2 != NULL);
> -    assert(ud2->string0 != NULL);
> -    assert(strcmp(ud2->string0, text) == 0);
> -    assert(ud2->dict1 == NULL);
> -
> -    /* confirm & release construction error */
> +    /* verify that visit_type_XXX() cleans up properly on error */
>      error_free_or_abort(&err);
> +    assert(!ud2);
> +
> +    /* Manually create a partial object, leaving ud2->dict1 at NULL */
> +    ud2 = g_new0(UserDefTwo, 1);
> +    ud2->string0 = g_strdup(text);
>
>      /* tear down partial object */
>      qapi_free_UserDefTwo(ud2);

Somewhat redundant now, because the free hidden in the failed visit
already covers partial object teardown.  I don't mind.

> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index d71727e..e039455 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -182,10 +182,7 @@ static void test_validate_fail_struct(TestInputVisitorData *data,
>
>      visit_type_TestStruct(v, NULL, &p, &err);
>      error_free_or_abort(&err);
> -    if (p) {
> -        g_free(p->string);
> -    }
> -    g_free(p);
> +    g_assert(!p);
>  }
>
>  static void test_validate_fail_struct_nested(TestInputVisitorData *data,
> @@ -199,7 +196,7 @@ static void test_validate_fail_struct_nested(TestInputVisitorData *data,
>
>      visit_type_UserDefTwo(v, NULL, &udp, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefTwo(udp);
> +    g_assert(!udp);
>  }
>
>  static void test_validate_fail_list(TestInputVisitorData *data,
> @@ -213,7 +210,7 @@ static void test_validate_fail_list(TestInputVisitorData *data,
>
>      visit_type_UserDefOneList(v, NULL, &head, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefOneList(head);
> +    g_assert(!head);
>  }
>
>  static void test_validate_fail_union_native_list(TestInputVisitorData *data,
> @@ -228,7 +225,7 @@ static void test_validate_fail_union_native_list(TestInputVisitorData *data,
>
>      visit_type_UserDefNativeListUnion(v, NULL, &tmp, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefNativeListUnion(tmp);
> +    g_assert(!tmp);
>  }
>
>  static void test_validate_fail_union_flat(TestInputVisitorData *data,
> @@ -242,7 +239,7 @@ static void test_validate_fail_union_flat(TestInputVisitorData *data,
>
>      visit_type_UserDefFlatUnion(v, NULL, &tmp, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefFlatUnion(tmp);
> +    g_assert(!tmp);
>  }
>
>  static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
> @@ -257,13 +254,13 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data,
>
>      visit_type_UserDefFlatUnion2(v, NULL, &tmp, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefFlatUnion2(tmp);
> +    g_assert(!tmp);
>  }
>
>  static void test_validate_fail_alternate(TestInputVisitorData *data,
>                                           const void *unused)
>  {
> -    UserDefAlternate *tmp = NULL;
> +    UserDefAlternate *tmp;

Did this initialization become dead in PATCH 03 already?

>      Visitor *v;
>      Error *err = NULL;
>
> @@ -271,7 +268,7 @@ static void test_validate_fail_alternate(TestInputVisitorData *data,
>
>      visit_type_UserDefAlternate(v, NULL, &tmp, &err);
>      error_free_or_abort(&err);
> -    qapi_free_UserDefAlternate(tmp);
> +    g_assert(!tmp);
>  }
>
>  static void do_test_validate_qmp_introspect(TestInputVisitorData *data,
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index ac8ebea..0b4153a 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -760,18 +760,12 @@ static void test_visitor_in_errors(TestInputVisitorData *data,
>
>      visit_type_TestStruct(v, NULL, &p, &err);
>      error_free_or_abort(&err);
> -    /* FIXME - a failed parse should not leave a partially-allocated p
> -     * for us to clean up; this could cause callers to leak memory. */
> -    g_assert(p->string == NULL);
> -
> -    g_free(p->string);
> -    g_free(p);
> +    g_assert(!p);
>
>      v = visitor_input_test_init(data, "[ '1', '2', false, '3' ]");
>      visit_type_strList(v, NULL, &q, &err);
>      error_free_or_abort(&err);
> -    assert(q);
> -    qapi_free_strList(q);
> +    assert(!q);
>  }
>
>  static void test_visitor_in_wrong_type(TestInputVisitorData *data,
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 99080db..3b2a27f 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -904,7 +904,10 @@ Example:
>          }
>          visit_check_struct(v, &err);
>      out_obj:
> -        visit_end_struct(v);
> +        if (visit_end_struct(v) && err) {
> +            qapi_free_UserDefOne(*obj);
> +            *obj = NULL;
> +        }
>      out:
>          error_propagate(errp, err);
>      }
> @@ -927,7 +930,10 @@ Example:
>              }
>          }
>
> -        visit_end_list(v);
> +        if (visit_end_list(v) && err) {
> +            qapi_free_UserDefOneList(*obj);
> +            *obj = NULL;
> +        }
>      out:
>          error_propagate(errp, err);
>      }