Eric Blake <ebl...@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 <ebl...@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);
> }
