Message ID | 1453219845-30939-36-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
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). > > Since input visitors have blind assignment semantics, we have to > track the result of whether an assignment is made all the way down > to each visitor callback implementation, to avoid making decisions > based on potentially uninitialized storage. I'm not sure I get this paragraph. What's "blind assignment semantics"? > 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). The assigning input visitor functions (core and generated) all assign either a pointer to a newly allocated object, or a non-pointer scalar value. Possible behaviors on error: (0) What we have now: assign something that must be cleaned up with the dealloc visitor if it's a pointer, but is otherwise useless CON: callers have to clean up CON: exposes careless callers to useless values (1) Don't assign anything PRO: consistent CON: exposes careless callers to uninitialized values (2) Assign zero bits PRO: consistent CON: exposes careless callers to bogus zero values (3) Assign null pointer, else don't assign anything CON: inconsistent CON: mix of (1)'s and (2)'s CON (4) Other ideas? Note that *obj is almost always null on entry, because we allocate objects zero-initialized. Only root visits can expose their caller to uninitialized values. > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > 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-impl.h | 6 ++--- > include/qapi/visitor.h | 53 ++++++++++++++++++++++++++++-------------- > qapi/opts-visitor.c | 11 ++++++--- > qapi/qapi-dealloc-visitor.c | 9 ++++--- > qapi/qapi-visit-core.c | 45 ++++++++++++++++++++++++++++------- > qapi/qmp-input-visitor.c | 18 +++++++++----- > qapi/qmp-output-visitor.c | 6 +++-- > qapi/string-input-visitor.c | 6 +++-- > qapi/string-output-visitor.c | 3 ++- > scripts/qapi-visit.py | 40 +++++++++++++++++++++++-------- > tests/test-qmp-commands.c | 13 +++++------ > tests/test-qmp-input-strict.c | 19 +++++++-------- > tests/test-qmp-input-visitor.c | 10 ++------ > 13 files changed, 157 insertions(+), 82 deletions(-) > > diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h > index acbe7d6..8df4ba1 100644 > --- a/include/qapi/visitor-impl.h > +++ b/include/qapi/visitor-impl.h > @@ -26,7 +26,7 @@ struct Visitor > { > /* Must be provided to visit structs (the string visitors do not > * currently visit structs). */ > - void (*start_struct)(Visitor *v, const char *name, void **obj, > + bool (*start_struct)(Visitor *v, const char *name, void **obj, > size_t size, Error **errp); > /* May be NULL; most useful for input visitors. */ > void (*check_struct)(Visitor *v, Error **errp); > @@ -34,13 +34,13 @@ struct Visitor > void (*end_struct)(Visitor *v); > > /* May be NULL; most useful for input visitors. */ > - void (*start_implicit_struct)(Visitor *v, void **obj, size_t size, > + bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size, > Error **errp); > /* May be NULL */ > void (*end_implicit_struct)(Visitor *v); > > /* Must be set */ > - void (*start_list)(Visitor *v, const char *name, GenericList **list, > + bool (*start_list)(Visitor *v, const char *name, GenericList **list, > Error **errp); > /* Must be set */ > GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp); > diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h > index 4638863..4eae633 100644 > --- a/include/qapi/visitor.h > +++ b/include/qapi/visitor.h > @@ -31,6 +31,27 @@ > * visitor-impl.h. > */ > > +/* All qapi types have a corresponding function with a signature > + * roughly compatible with this: > + * > + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp); > + * > + * where *@obj is itself a pointer or a scalar. The visit functions for > + * built-in types are declared here, while the functions for qapi-defined > + * struct, union, enum, and list types are generated (see qapi-visit.h). > + * Input visitors may receive an uninitialized *@obj, and guarantee a > + * safe value is assigned (a new object on success, or NULL on failure). This specifies the safe value: NULL. Makes no sense for non-pointer scalars. May input visitors really receive uninitialized *@obj? As far as I can see, we routinely store a newly allocated object to *@obj on success, and store nothing on failure. To be able to pass this to the dealloc visitor, *@obj must have been null initially, mustn't it? > + * Output visitors expect *@obj to be properly initialized on entry. What about the dealloc visitor? > + * > + * Additionally, all qapi structs have a generated function compatible > + * with this: > + * > + * void qapi_free_FOO(void *obj); > + * > + * which behaves like free(), even if @obj is NULL or was only partially > + * allocated before encountering an error. Will partially allocated QAPI objects remain visible outside input visitors? > + */ > + > /* This struct is layout-compatible with all other *List structs > * created by the qapi generator. */ > typedef struct GenericList > @@ -62,11 +83,12 @@ typedef struct GenericList > * Set *@errp on failure; for example, if the input stream does not > * have a member @name or if the member is not an object. > * > - * FIXME: For input visitors, *@obj can be assigned here even if later > - * visits will fail; this can lead to memory leaks if clients aren't > - * careful. > + * Returns true if *@obj was allocated; if that happens, and an error > + * occurs any time before the matching visit_end_struct(), then the > + * caller (usually a visit_type_FOO() function) knows to undo the > + * allocation before returning control further. > */ > -void visit_start_struct(Visitor *v, const char *name, void **obj, > +bool visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp); Need to see how this is used before I can judge whether I like it :) > /** > * Prepare for completing a struct visit. > @@ -85,19 +107,15 @@ void visit_end_struct(Visitor *v); > > /** > * Prepare to visit an implicit struct. > - * Similar to visit_start_struct(), except that an implicit struct > - * represents a subset of keys that are present at the same nesting level > - * of a common object but as a separate qapi C struct, rather than a new > - * object at a deeper nesting level. > + * Similar to visit_start_struct(), including return semantics, except > + * that an implicit struct represents a subset of keys that are present > + * at the same nesting level of a common object but as a separate qapi > + * C struct, rather than a new object at a deeper nesting level. > * > * @obj must not be NULL, since this function is only called when > * visiting with qapi structs. > - * > - * FIXME: For input visitors, *@obj can be assigned here even if later > - * visits will fail; this can lead to memory leaks if clients aren't > - * careful. > */ > -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size, > +bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size, > Error **errp); > /** > * Complete an implicit struct visit started earlier. > @@ -126,11 +144,12 @@ void visit_end_implicit_struct(Visitor *v); > * in this case, visit_next_list() will not be needed, but > * visit_end_list() is still mandatory. > * > - * FIXME: For input visitors, *@list can be assigned here even if > - * later visits will fail; this can lead to memory leaks if clients > - * aren't careful. > + * Returns true if *@list was allocated; if that happens, and an error > + * occurs any time before the matching visit_end_list(), then the > + * caller (usually a visit_type_FOO() function) knows to undo the > + * allocation before returning control further. > */ > -void visit_start_list(Visitor *v, const char *name, GenericList **list, > +bool visit_start_list(Visitor *v, const char *name, GenericList **list, > Error **errp); > /** > * Iterate over a GenericList during a list visit. > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index c5a7396..38d1e68 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -122,18 +122,20 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) > } > > > -static void > +static bool > opts_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > OptsVisitor *ov = to_ov(v); > const QemuOpt *opt; > + bool result = false; > > if (obj) { > *obj = g_malloc0(size > 0 ? size : 1); > + result = true; > } > if (ov->depth++ > 0) { > - return; > + return result; > } > > ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, > @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > ov->fake_id_opt->str = g_strdup(ov->opts_root->id); > opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); > } > + return result; > } Stores the newly allocated object in *@obj on success, doesn't store anything on failure. To make cleanup possible, *@obj must be null initially. Then the return value is true iff *@obj is non-null on return. > > > @@ -210,7 +213,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) > } > > > -static void > +static bool > opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > OptsVisitor *ov = to_ov(v); > @@ -223,8 +226,10 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > if (ov->repeated_opts) { > ov->list_mode = LM_IN_PROGRESS; > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > + return false; > } > } This one stores null on failure, unlike opts_start_struct(). Again, the return value is true iff *@list is non-null on return. > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index 839049e..0990199 100644 > --- a/qapi/qapi-dealloc-visitor.c > +++ b/qapi/qapi-dealloc-visitor.c > @@ -53,11 +53,12 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov) > return value; > } > > -static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, > +static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, > size_t unused, Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, obj); > + return false; > } > Not an input visitor, always returns false. > static void qapi_dealloc_end_struct(Visitor *v) > @@ -69,13 +70,14 @@ static void qapi_dealloc_end_struct(Visitor *v) > } > } > > -static void qapi_dealloc_start_implicit_struct(Visitor *v, > +static bool qapi_dealloc_start_implicit_struct(Visitor *v, > void **obj, > size_t size, > Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, obj); > + return false; > } > Likewise. > static void qapi_dealloc_end_implicit_struct(Visitor *v) > @@ -87,11 +89,12 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v) > } > } > > -static void qapi_dealloc_start_list(Visitor *v, const char *name, > +static bool qapi_dealloc_start_list(Visitor *v, const char *name, > GenericList **list, Error **errp) > { > QapiDeallocVisitor *qov = to_qov(v); > qapi_dealloc_push(qov, NULL); > + return false; > } > Likewise. > static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index f391a70..259e0cb 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -2,7 +2,7 @@ > * Core Definitions for QAPI Visitor Classes > * > * Copyright IBM, Corp. 2011 > - * Copyright (C) 2015 Red Hat, Inc. > + * Copyright (C) 2015-2016 Red Hat, Inc. > * > * Authors: > * Anthony Liguori <aliguori@us.ibm.com> > @@ -26,14 +26,20 @@ static bool visit_is_output(Visitor *v) > return v->type_enum == output_type_enum; > } > > -void visit_start_struct(Visitor *v, const char *name, void **obj, > +bool visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > + bool result; > + > assert(obj ? size : !size); > if (obj && visit_is_output(v)) { > assert(*obj); > } > - v->start_struct(v, name, obj, size, errp); > + result = v->start_struct(v, name, obj, size, errp); > + if (result) { > + assert(obj && *obj); > + } Roundabout way to write assert(!result || (obj && *obj)); Heh, you even assert one half of what I'm trying to prove. Can we make it assert(result == (visit_is_input(v) && obj && *obj) ? Similarly for the other functions. > + return result; > } > > void visit_check_struct(Visitor *v, Error **errp) > @@ -48,16 +54,22 @@ void visit_end_struct(Visitor *v) > v->end_struct(v); > } > > -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size, > +bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size, > Error **errp) > { > + bool result = false; > + > assert(obj && size); > if (visit_is_output(v)) { > assert(*obj); > } > if (v->start_implicit_struct) { > - v->start_implicit_struct(v, obj, size, errp); > + result = v->start_implicit_struct(v, obj, size, errp); > } > + if (result) { > + assert(*obj); > + } > + return result; > } > > void visit_end_implicit_struct(Visitor *v) > @@ -67,10 +79,14 @@ void visit_end_implicit_struct(Visitor *v) > } > } > > -void visit_start_list(Visitor *v, const char *name, GenericList **list, > +bool visit_start_list(Visitor *v, const char *name, GenericList **list, > Error **errp) > { > - v->start_list(v, name, list, errp); > + bool result = v->start_list(v, name, list, errp); > + if (result) { > + assert(list && *list); > + } > + return result; > } > > GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp) > @@ -229,6 +245,7 @@ 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: > @@ -236,7 +253,11 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) > assert(*obj); > } > */ > - v->type_str(v, name, obj, errp); > + v->type_str(v, name, obj, &err); > + if (!visit_is_output(v) && err) { > + *obj = NULL; Shouldn't storing null on error be left to v->type_str(), like we do for structs and lists? Not least because it begs the question whether this leaks a string stored by it. !visit_is_output(v) covers input visitors and the dealloc visitor. > + } > + error_propagate(errp, err); > } > > void visit_type_number(Visitor *v, const char *name, double *obj, > @@ -248,11 +269,17 @@ 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); > if (visit_is_output(v)) { > assert(*obj); > } > - v->type_any(v, name, obj, errp); > + v->type_any(v, name, obj, &err); > + if (!visit_is_output(v) && err) { > + *obj = NULL; Likewise. > + } > + error_propagate(errp, err); > } > > void visit_type_null(Visitor *v, const char *name, Error **errp) > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 82f9333..6b4ad68 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -130,7 +130,7 @@ static void qmp_input_pop(Visitor *v) > qiv->nb_stack--; > } > > -static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > +static bool qmp_input_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -140,30 +140,34 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > - return; > + return false; > } > > qmp_input_push(qiv, qobj, NULL, &err); > if (err) { > error_propagate(errp, err); > - return; > + return false; > } > > if (obj) { > *obj = g_malloc0(size); > + return true; > } > + return false; Stores the newly allocated object in *@obj on success, doesn't store anything on failure. To make cleanup possible, *@obj must be null initially. Then the return value is true iff *@obj is non-null on return. Just like opts-visitor.c. > } > > > -static void qmp_input_start_implicit_struct(Visitor *v, void **obj, > +static bool qmp_input_start_implicit_struct(Visitor *v, void **obj, > size_t size, Error **errp) > { > if (obj) { > *obj = g_malloc0(size); > + return true; > } > + return false; > } Likewise. > > -static void qmp_input_start_list(Visitor *v, const char *name, > +static bool qmp_input_start_list(Visitor *v, const char *name, > GenericList **list, Error **errp) > { > QmpInputVisitor *qiv = to_qiv(v); > @@ -173,7 +177,7 @@ static void qmp_input_start_list(Visitor *v, const char *name, > if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "list"); > - return; > + return false; > } > > entry = qlist_first(qobject_to_qlist(qobj)); > @@ -181,10 +185,12 @@ static void qmp_input_start_list(Visitor *v, const char *name, > if (list) { > if (entry) { > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > } > } > + return false; > } This one stores null on failure, unlike start_struct(). Again, the return value is true iff *@list is non-null on return. Just like opts-visitor.c. > > static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index 913f378..ce592d2 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -102,7 +102,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, > } > } > > -static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, > +static bool qmp_output_start_struct(Visitor *v, const char *name, void **obj, > size_t unused, Error **errp) > { > QmpOutputVisitor *qov = to_qov(v); > @@ -110,6 +110,7 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, > > qmp_output_add(qov, name, dict); > qmp_output_push(qov, dict); > + return false; > } Not an input visitor, always returns false. > > static void qmp_output_end_struct(Visitor *v) > @@ -118,7 +119,7 @@ static void qmp_output_end_struct(Visitor *v) > qmp_output_pop(qov, QTYPE_QDICT); > } > > -static void qmp_output_start_list(Visitor *v, const char *name, > +static bool qmp_output_start_list(Visitor *v, const char *name, > GenericList **listp, Error **errp) > { > QmpOutputVisitor *qov = to_qov(v); > @@ -126,6 +127,7 @@ static void qmp_output_start_list(Visitor *v, const char *name, > > qmp_output_add(qov, name, list); > qmp_output_push(qov, list); > + return false; > } Likewise. > > static GenericList *qmp_output_next_list(Visitor *v, GenericList *list, > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index 582a62a..0e4a07c 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -120,7 +120,7 @@ error: > siv->ranges = NULL; > } > > -static void > +static bool > start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > StringInputVisitor *siv = to_siv(v); > @@ -132,7 +132,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > parse_str(siv, &err); > if (err) { > error_propagate(errp, err); > - return; > + return false; > } > > siv->cur_range = g_list_first(siv->ranges); > @@ -142,8 +142,10 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > siv->cur = r->begin; > } > *list = g_new0(GenericList, 1); > + return true; > } else { > *list = NULL; > + return false; > } > } This one stores null on failure, like the start_list() we've seen before, and unlike the start_struct(). Again, the return value is true iff *@list is non-null on return. > > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index 7209d80..2666802 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -263,7 +263,7 @@ static void print_type_number(Visitor *v, const char *name, double *obj, > string_output_set(sov, g_strdup_printf("%f", *obj)); > } > > -static void > +static bool > start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > { > StringOutputVisitor *sov = to_sov(v); > @@ -276,6 +276,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) > if (*list && (*list)->next) { > sov->list_mode = LM_STARTED; > } > + return false; > } Not an input visitor, always returns false. > > static GenericList * > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 6016734..eebb5f7 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ): > static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp) > { > Error *err = NULL; > + bool allocated; > > - visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err); > + allocated = visit_start_implicit_struct(v, (void **)obj, > + sizeof(%(c_type)s), &err); > if (err) { > goto out; > } > @@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * > visit_type_%(c_type)s_fields(v, obj, &err); > out_obj: > visit_end_implicit_struct(v); > + if (allocated && err) { > + g_free(*obj); > + *obj = NULL; > + } > out: > error_propagate(errp, err); > } > @@ -116,18 +122,15 @@ 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) > { > Error *err = NULL; > %(c_name)s *elt; > + bool allocated; > > - visit_start_list(v, name, (GenericList **)obj, &err); > + allocated = visit_start_list(v, name, (GenericList **)obj, &err); > if (err) { > goto out; > } > @@ -144,6 +147,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > } > visit_end_list(v); > out: > + if (allocated && err) { > + qapi_free_%(c_name)s(*obj); > + *obj = NULL; > + } > error_propagate(errp, err); > } > ''', > @@ -174,8 +181,10 @@ def gen_visit_alternate(name, variants): > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) > { > Error *err = NULL; > + bool allocated; > > - visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err); > + allocated = visit_start_implicit_struct(v, (void **)obj, > + sizeof(%(c_name)s), &err); > if (err) { > goto out; > } > @@ -204,11 +213,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error > } > out_obj: > visit_end_implicit_struct(v); > + if (allocated && 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 > > @@ -236,8 +249,10 @@ def gen_visit_object(name, base, members, variants): > void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) > { > Error *err = NULL; > + bool allocated; > > - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); > + allocated = visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), > + &err); > if (err) { > goto out; > } > @@ -301,10 +316,15 @@ 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 (allocated && err) { > + qapi_free_%(c_name)s(*obj); > + *obj = NULL; > + } > out: > error_propagate(errp, err); > } > -''') > +''', > + c_name=c_name(name)) > > return ret > Hmm. This grows qapi-visit.c by 14% for me. Can we do the deallocation in the visitor core somehow? We'd have to pass "how to deallocate" information: the appropriate qapi_free_FOO() function. We already pass in "how to allocate" information: size. > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > index bc8085d..d3466a4 100644 > --- a/tests/test-qmp-commands.c > +++ b/tests/test-qmp-commands.c > @@ -227,14 +227,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); > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index 775ad39..4db35dd 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -181,10 +181,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, > @@ -198,7 +195,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, > @@ -212,7 +209,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, > @@ -227,7 +224,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, > @@ -241,7 +238,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, > @@ -256,13 +253,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; Are you sure this is sound? > Visitor *v; > Error *err = NULL; > > @@ -270,7 +267,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 cd5e765..0eef7e0 100644 > --- a/tests/test-qmp-input-visitor.c > +++ b/tests/test-qmp-input-visitor.c > @@ -733,18 +733,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, Now I've seen the complete patch, two more remarks: * I think all the allocating methods should behave the same. Right now, some store null on failure, and some don't. For the latter to work, the value must be null initially (or else the dealloc visitor runs amok). * Do we really need the new return value? It looks like it's always equal to visit_is_input(v) && obj && *obj.
On 01/28/2016 08:24 AM, Markus Armbruster wrote: > 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). >> >> Since input visitors have blind assignment semantics, we have to >> track the result of whether an assignment is made all the way down >> to each visitor callback implementation, to avoid making decisions >> based on potentially uninitialized storage. > > I'm not sure I get this paragraph. What's "blind assignment semantics"? The caller does: { Foo *bar; /* uninit */ visit_type_Foo(&bar); if (no error) { /* use bar */ } } Which means the visitor core can't do 'if (*obj)', because obj may be uninitialized on entry; if it dereferences obj at all, it must be via '*obj = ...' which I'm terming a blind assignment. But I can try and come up with better wording. > >> 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). > > The assigning input visitor functions (core and generated) all assign > either a pointer to a newly allocated object, or a non-pointer scalar > value. > > Possible behaviors on error: > > (0) What we have now: assign something that must be cleaned up with the > dealloc visitor if it's a pointer, but is otherwise useless > > CON: callers have to clean up > CON: exposes careless callers to useless values > > (1) Don't assign anything > > PRO: consistent > CON: exposes careless callers to uninitialized values Half-PRO: Caller can pre-initialize a default, and rely on that value on error. In fact, I think we have callers doing that when visiting an enum, and I didn't feel up to auditing them all when first writing the patch. But a small audit right now shows: qom/object.c:object_property_get_enum() starts with uninitialized 'int ret;', hardcodes 'return 0;' on some failures, but otherwise passes it to visit_type_enum() then blindly returns that value even if errp is set. Yuck. Callers HAVE to check errp rather than relying on the return value to flag errors; although it looks like the lone caller is in numa.c and passes &error_abort. Maybe I should just bite the bullet, and audit ALL uses of visitor for their behavior of what to expect in *obj on error. > > (2) Assign zero bits > > PRO: consistent > CON: exposes careless callers to bogus zero values Half-CON: Caller cannot pre-initialize a default > > (3) Assign null pointer, else don't assign anything > > CON: inconsistent > CON: mix of (1)'s and (2)'s CON Which I think is what I did in this patch. > > (4) Other ideas? Store the caller's initial value (often all zero, but not necessarily), and restore THAT value on error (preserves a pre-initialized default, but leaves uninitialized garbage in place to bite careless callers) > > Note that *obj is almost always null on entry, because we allocate > objects zero-initialized. Only root visits can expose their caller to > uninitialized values. > >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> +/* All qapi types have a corresponding function with a signature >> + * roughly compatible with this: >> + * >> + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp); >> + * >> + * where *@obj is itself a pointer or a scalar. The visit functions for >> + * built-in types are declared here, while the functions for qapi-defined >> + * struct, union, enum, and list types are generated (see qapi-visit.h). >> + * Input visitors may receive an uninitialized *@obj, and guarantee a >> + * safe value is assigned (a new object on success, or NULL on failure). > > This specifies the safe value: NULL. Makes no sense for non-pointer > scalars. > > May input visitors really receive uninitialized *@obj? As far as I can > see, we routinely store a newly allocated object to *@obj on success, > and store nothing on failure. To be able to pass this to the dealloc > visitor, *@obj must have been null initially, mustn't it? Correct. Pre-patch: either the caller pre-initialized obj to NULL (and can then blindly pass it to the dealloc visitor regardless of whether visit_start_struct() succeeded), or it did not (in which case the dealloc visitor must not be called if *obj remains uninitialized because visit_start_struct() failed, but MUST be called if visit_start_struct() succeeded to clean up the partial object). Post-patch: calling visit_start_struct() always assigns to *obj, and the dealloc visitor can be safely called. > >> + * Output visitors expect *@obj to be properly initialized on entry. > > What about the dealloc visitor? Can be passed NULL, a partial object, or a complete object. But spelling that out would indeed be useful. > >> + * >> + * Additionally, all qapi structs have a generated function compatible >> + * with this: >> + * >> + * void qapi_free_FOO(void *obj); >> + * >> + * which behaves like free(), even if @obj is NULL or was only partially >> + * allocated before encountering an error. > > Will partially allocated QAPI objects remain visible outside input > visitors? A user can still hand-initialize a QAPI struct into partial state, although you are correct that this patch is what is changing things to no longer leak a partial object outside of the visit_type_FOO() calls. >> + * Returns true if *@obj was allocated; if that happens, and an error >> + * occurs any time before the matching visit_end_struct(), then the >> + * caller (usually a visit_type_FOO() function) knows to undo the >> + * allocation before returning control further. >> */ >> -void visit_start_struct(Visitor *v, const char *name, void **obj, >> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp); > > Need to see how this is used before I can judge whether I like it :) > >> @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, >> ov->fake_id_opt->str = g_strdup(ov->opts_root->id); >> opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); >> } >> + return result; >> } > > Stores the newly allocated object in *@obj on success, doesn't store > anything on failure. > > To make cleanup possible, *@obj must be null initially. Then the return > value is true iff *@obj is non-null on return. If we want, I can change these to all store *obj = NULL on failure. Thinking about it more: for any given visit_type_FOO(), if visit_start_struct() succeeds but something else later fails, *obj will be NULL; so it also makes sense that if visit_start_struct() fails than *obj should be NULL. >> -void visit_start_struct(Visitor *v, const char *name, void **obj, >> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >> size_t size, Error **errp) >> { >> + bool result; >> + >> assert(obj ? size : !size); >> if (obj && visit_is_output(v)) { >> assert(*obj); >> } >> - v->start_struct(v, name, obj, size, errp); >> + result = v->start_struct(v, name, obj, size, errp); >> + if (result) { >> + assert(obj && *obj); >> + } > > Roundabout way to write assert(!result || (obj && *obj)); > > Heh, you even assert one half of what I'm trying to prove. > > Can we make it assert(result == (visit_is_input(v) && obj && *obj) ? Missing a ); guessing that you meant: assert(result == (visit_is_input(v) && obj && *obj)); Yes, I think we can, but we'd need a visit_is_input() helper. >> @@ -236,7 +253,11 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) >> assert(*obj); >> } >> */ >> - v->type_str(v, name, obj, errp); >> + v->type_str(v, name, obj, &err); >> + if (!visit_is_output(v) && err) { >> + *obj = NULL; > > Shouldn't storing null on error be left to v->type_str(), like we do for > structs and lists? Not least because it begs the question whether this > leaks a string stored by it. May be worthwhile to mandate that tighter semantics on each visitor. >> +++ b/scripts/qapi-visit.py >> @@ -301,10 +316,15 @@ 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 (allocated && err) { >> + qapi_free_%(c_name)s(*obj); >> + *obj = NULL; >> + } >> out: >> error_propagate(errp, err); >> } >> -''') >> +''', >> + c_name=c_name(name)) >> >> return ret >> > > Hmm. > > This grows qapi-visit.c by 14% for me. > > Can we do the deallocation in the visitor core somehow? We'd have to > pass "how to deallocate" information: the appropriate qapi_free_FOO() > function. We already pass in "how to allocate" information: size. I thought about that; do we really want to be type-punning functions like that? But it does sound smaller; I can play with the idea. > > Now I've seen the complete patch, two more remarks: > > * I think all the allocating methods should behave the same. Right now, > some store null on failure, and some don't. For the latter to work, > the value must be null initially (or else the dealloc visitor runs > amok). Agree; I'm leaning towards ALL allocating methods must store into *obj (either NULL on failure, or object on success). > > * Do we really need the new return value? It looks like it's always > equal to visit_is_input(v) && obj && *obj. Except we don't (yet) have a visit_is_input() function, let alone one visible from within the generated qapi-visit.c code. Maybe doing that first would help.
Eric Blake <eblake@redhat.com> writes: > On 01/28/2016 08:24 AM, Markus Armbruster wrote: >> 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). >>> >>> Since input visitors have blind assignment semantics, we have to >>> track the result of whether an assignment is made all the way down >>> to each visitor callback implementation, to avoid making decisions >>> based on potentially uninitialized storage. >> >> I'm not sure I get this paragraph. What's "blind assignment semantics"? > > The caller does: > > { > Foo *bar; /* uninit */ > visit_type_Foo(&bar); > if (no error) { > /* use bar */ > } > } > > Which means the visitor core can't do 'if (*obj)', because obj may be > uninitialized on entry; if it dereferences obj at all, it must be via > '*obj = ...' which I'm terming a blind assignment. > > But I can try and come up with better wording. I'd suggest one, but I think we should first make up our minds on error behavior. >>> 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). >> >> The assigning input visitor functions (core and generated) all assign >> either a pointer to a newly allocated object, or a non-pointer scalar >> value. >> >> Possible behaviors on error: >> >> (0) What we have now: assign something that must be cleaned up with the >> dealloc visitor if it's a pointer, but is otherwise useless >> >> CON: callers have to clean up >> CON: exposes careless callers to useless values >> >> (1) Don't assign anything Need to be very careful to store only when success is assured. Consider visiting a QAPI type that is actually two levels of containers, say a list of a struct of scalars. The visit is a walk of this tree then: list ___/ ... \___ / \ struct1 structN / ... \ / ... \ scal11 scal1M scalN1 scalNM Now let's consider the state when the visit reaches scalN1: * The visits of scal11..scal1M and struct 1 have all succeeded already, and stored their value into their container. Same for the visits of structI (I < N) omitted in the diagram, and their scalars. * The visit of list and struct N are in progress: the object has been partially constructed, but not yet stored. * The remaining visits haven't begun, and their members in objects already allocated are still zero. Now the visit of scalN1 fails. The visit of structN fails in turn, freeing its (not yet stored, partially constructed) object. Finally, the visit of list fails, freeing its object. That the failed visits of scalN1 and structN didn't store is actually unimportant. Of course, this isn't how things work now. Visits of containers store the newly allocated object before visiting members, in visit_start_*(), and the member visits use that to find the object. >> PRO: consistent >> CON: exposes careless callers to uninitialized values > > Half-PRO: Caller can pre-initialize a default, and rely on that value on > error. In fact, I think we have callers doing that when visiting an > enum, and I didn't feel up to auditing them all when first writing the > patch. > > But a small audit right now shows: > > qom/object.c:object_property_get_enum() starts with uninitialized 'int > ret;', hardcodes 'return 0;' on some failures, but otherwise passes it > to visit_type_enum() then blindly returns that value even if errp is > set. Yuck. Callers HAVE to check errp rather than relying on the > return value to flag errors; although it looks like the lone caller is > in numa.c and passes &error_abort. > > Maybe I should just bite the bullet, and audit ALL uses of visitor for > their behavior of what to expect in *obj on error. >> >> (2) Assign zero bits >> >> PRO: consistent >> CON: exposes careless callers to bogus zero values > > Half-CON: Caller cannot pre-initialize a default With (1) don't assign, the caller can pick an error value by assigning it before the visit, and it must not access the value on error unless it does. With (2) assign zero, the caller can't pick an error value, but may safely access the value even on error. Tradeoff. I figure either can work for us. >> (3) Assign null pointer, else don't assign anything >> >> CON: inconsistent >> CON: mix of (1)'s and (2)'s CON > > Which I think is what I did in this patch. I don't like the inconsistency. It complicates the interface. >> (4) Other ideas? > > Store the caller's initial value (often all zero, but not necessarily), > and restore THAT value on error (preserves a pre-initialized default, > but leaves uninitialized garbage in place to bite careless callers) Behaves like (1). Under the hood, it replaces "store only when success is assured" by "undo the store on failure". I'd guess this would be more complicated. >> Note that *obj is almost always null on entry, because we allocate >> objects zero-initialized. Only root visits can expose their caller to >> uninitialized values. >> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> > >>> +/* All qapi types have a corresponding function with a signature >>> + * roughly compatible with this: >>> + * >>> + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp); >>> + * >>> + * where *@obj is itself a pointer or a scalar. The visit functions for >>> + * built-in types are declared here, while the functions for qapi-defined >>> + * struct, union, enum, and list types are generated (see qapi-visit.h). >>> + * Input visitors may receive an uninitialized *@obj, and guarantee a >>> + * safe value is assigned (a new object on success, or NULL on failure). >> >> This specifies the safe value: NULL. Makes no sense for non-pointer >> scalars. >> >> May input visitors really receive uninitialized *@obj? As far as I can >> see, we routinely store a newly allocated object to *@obj on success, >> and store nothing on failure. To be able to pass this to the dealloc >> visitor, *@obj must have been null initially, mustn't it? > > Correct. Pre-patch: either the caller pre-initialized obj to NULL (and > can then blindly pass it to the dealloc visitor regardless of whether > visit_start_struct() succeeded), Yes. Three cases: (a) visit_start_struct() succeeds: it stored to *obj, and caller must clean it up. (b) visit_start_struct() fails without storing to *obj: *obj is still null, and caller may clean it up (it's a nop). Whatever visit_start_struct() allocated, it has to clean up itself. (c) visit_start_struct() fails with storing to *obj: caller must clean it up. Cleanup is relatively easy for the caller: just do it always. But what if caller forgets to initialize to null? That's next: > or it did not (in which case the > dealloc visitor must not be called if *obj remains uninitialized because > visit_start_struct() failed, but MUST be called if visit_start_struct() > succeeded to clean up the partial object). Case (b) changes: (b) visit_start_struct() fails without storing to *obj: caller must not use *obj. Whatever visit_start_struct() allocated, it has to clean up itself. I can't see how the caller could get cleanup right without initializing to null. > Post-patch: calling visit_start_struct() always assigns to *obj, and the > dealloc visitor can be safely called. Actually, it always assigns *pointers*. That's enough to permit unconditional cleanup, of course. >>> + * Output visitors expect *@obj to be properly initialized on entry. >> >> What about the dealloc visitor? > > Can be passed NULL, a partial object, or a complete object. But > spelling that out would indeed be useful. > >> >>> + * >>> + * Additionally, all qapi structs have a generated function compatible >>> + * with this: >>> + * >>> + * void qapi_free_FOO(void *obj); >>> + * >>> + * which behaves like free(), even if @obj is NULL or was only partially >>> + * allocated before encountering an error. >> >> Will partially allocated QAPI objects remain visible outside input >> visitors? > > A user can still hand-initialize a QAPI struct into partial state, > although you are correct that this patch is what is changing things to > no longer leak a partial object outside of the visit_type_FOO() calls. Okay. >>> + * Returns true if *@obj was allocated; if that happens, and an error >>> + * occurs any time before the matching visit_end_struct(), then the >>> + * caller (usually a visit_type_FOO() function) knows to undo the >>> + * allocation before returning control further. >>> */ >>> -void visit_start_struct(Visitor *v, const char *name, void **obj, >>> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >>> size_t size, Error **errp); >> >> Need to see how this is used before I can judge whether I like it :) >> >>> @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, >>> ov->fake_id_opt->str = g_strdup(ov->opts_root->id); >>> opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); >>> } >>> + return result; >>> } >> >> Stores the newly allocated object in *@obj on success, doesn't store >> anything on failure. >> >> To make cleanup possible, *@obj must be null initially. Then the return >> value is true iff *@obj is non-null on return. > > If we want, I can change these to all store *obj = NULL on failure. > Thinking about it more: for any given visit_type_FOO(), if > visit_start_struct() succeeds but something else later fails, *obj will > be NULL; so it also makes sense that if visit_start_struct() fails than > *obj should be NULL. I think behavior (1) don't assign and (2) assign zero both work, we just have to pick one and run with it. If we pick behavior (1) don't assign, then we should assert something like !obj || !*obj on entry. With such assertions in place, I think (1) should be roughly as safe as (2). >>> -void visit_start_struct(Visitor *v, const char *name, void **obj, >>> +bool visit_start_struct(Visitor *v, const char *name, void **obj, >>> size_t size, Error **errp) >>> { >>> + bool result; >>> + >>> assert(obj ? size : !size); >>> if (obj && visit_is_output(v)) { >>> assert(*obj); >>> } >>> - v->start_struct(v, name, obj, size, errp); >>> + result = v->start_struct(v, name, obj, size, errp); >>> + if (result) { >>> + assert(obj && *obj); >>> + } >> >> Roundabout way to write assert(!result || (obj && *obj)); >> >> Heh, you even assert one half of what I'm trying to prove. >> >> Can we make it assert(result == (visit_is_input(v) && obj && *obj) ? > > Missing a ); guessing that you meant: > > assert(result == (visit_is_input(v) && obj && *obj)); Yes. > Yes, I think we can, but we'd need a visit_is_input() helper. Right now, I'm not proposing to change the assertion, I'm only trying to find out whether we actually need the return value. >>> @@ -236,7 +253,11 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) >>> assert(*obj); >>> } >>> */ >>> - v->type_str(v, name, obj, errp); >>> + v->type_str(v, name, obj, &err); >>> + if (!visit_is_output(v) && err) { >>> + *obj = NULL; >> >> Shouldn't storing null on error be left to v->type_str(), like we do for >> structs and lists? Not least because it begs the question whether this >> leaks a string stored by it. > > May be worthwhile to mandate that tighter semantics on each visitor. Yes, please. If we adopt consistent behaviors (1) or (2), a general rule will cover them all. >>> +++ b/scripts/qapi-visit.py > >>> @@ -301,10 +316,15 @@ 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 (allocated && err) { >>> + qapi_free_%(c_name)s(*obj); >>> + *obj = NULL; >>> + } >>> out: >>> error_propagate(errp, err); >>> } >>> -''') >>> +''', >>> + c_name=c_name(name)) >>> >>> return ret >>> >> >> Hmm. >> >> This grows qapi-visit.c by 14% for me. >> >> Can we do the deallocation in the visitor core somehow? We'd have to >> pass "how to deallocate" information: the appropriate qapi_free_FOO() >> function. We already pass in "how to allocate" information: size. > > I thought about that; do we really want to be type-punning functions > like that? But it does sound smaller; I can play with the idea. Type-punning functions is usually a bad idea. Calling the result has undefined behavior unless the two function types are compatible. The function types at hand differ only in the parameter type, but void * would not be compatible with some struct QAPIType *. As long as the two pointer types have the same representation, the compiler would have to work to actually screw this up. A perfectly clean solution would need a wrapper function, negating some of the code savings. >> Now I've seen the complete patch, two more remarks: >> >> * I think all the allocating methods should behave the same. Right now, >> some store null on failure, and some don't. For the latter to work, >> the value must be null initially (or else the dealloc visitor runs >> amok). > > Agree; I'm leaning towards ALL allocating methods must store into *obj > (either NULL on failure, or object on success). If this was a function return value, correct behavior would be obvious: return object on success, return null on failure. Equivalent to (2). But this is a side effect. There, I rather like "do nothing on failure". That's (1). But "rather like" doesn't justify implementation complexity. If (2) turns out to be simpler, go for it. >> * Do we really need the new return value? It looks like it's always >> equal to visit_is_input(v) && obj && *obj. > > Except we don't (yet) have a visit_is_input() function, let alone one > visible from within the generated qapi-visit.c code. Maybe doing that > first would help. If we move the destruction into the core, this problem goes away: void visit_type_T(Visitor *v, const char *name, T **obj, Error **errp) { Error *err = NULL; visit_start_struct(v, name, (void **)obj, sizeof(T), &err); if (err) { goto out; } if (!*obj) { goto out_obj; } visit_type_T_fields(v, obj, &err); if (err) { goto out_obj; } visit_check_struct(v, &err); out_obj: visit_end_struct(v, qapi_free_T); // type punning omitted here out: error_propagate(errp, err); } v->end_struct() frees if v is an input visitor and an error occured. The former condition is trivial, the latter requires tracking errors. Storing &err in the stack object could do. Alternatively, pass err through visit_end_struct() to v->end_struct(). Yet another way to skin this cat: instead of freeing, end_struct() returns whether to free. Looks like this: out_obj: if (visit_end_struct(v)) { qapi_free_T(*obj); } or maybe returns whether something was allocated: out_obj: if (visit_end_struct(v) && err) { qapi_free_T(*obj); } These are perhaps the simplest solutions so far.
On 01/29/2016 05:03 AM, Markus Armbruster wrote: > > With (1) don't assign, the caller can pick an error value by assigning > it before the visit, and it must not access the value on error unless it > does. > > With (2) assign zero, the caller can't pick an error value, but may > safely access the value even on error. > > Tradeoff. I figure either can work for us. > >>> (3) Assign null pointer, else don't assign anything >>> >>> CON: inconsistent >>> CON: mix of (1)'s and (2)'s CON >> >> Which I think is what I did in this patch. > > I don't like the inconsistency. It complicates the interface. I'll go ahead and audit to see whether more scalar visits were relying on (1) and would have to be rewritten to use style (2); vs. whether more pointer visits were passing in uninitialized obj and would have to be rewritten to use style (1). > I think behavior (1) don't assign and (2) assign zero both work, we just > have to pick one and run with it. > > If we pick behavior (1) don't assign, then we should assert something > like !obj || !*obj on entry. With such assertions in place, I think (1) > should be roughly as safe as (2). I think your assessment is right, and it's now just a matter of seeing which way to get to a consistent state is less effort (I may still end up doing both ways as competing patches, for comparison purposes). > or maybe returns whether something was allocated: > > out_obj: > if (visit_end_struct(v) && err) { > qapi_free_T(*obj); > } I'm liking that. Dealloc and output visitors always return false, and input visitors may need to track something on their stack for whether they allocated or returned error earlier on, but it results in less generated output. Basically, it's lowering the 'bool allocated' that I added in this attempt out of the generated code and into the visitors.
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index acbe7d6..8df4ba1 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -26,7 +26,7 @@ struct Visitor { /* Must be provided to visit structs (the string visitors do not * currently visit structs). */ - void (*start_struct)(Visitor *v, const char *name, void **obj, + bool (*start_struct)(Visitor *v, const char *name, void **obj, size_t size, Error **errp); /* May be NULL; most useful for input visitors. */ void (*check_struct)(Visitor *v, Error **errp); @@ -34,13 +34,13 @@ struct Visitor void (*end_struct)(Visitor *v); /* May be NULL; most useful for input visitors. */ - void (*start_implicit_struct)(Visitor *v, void **obj, size_t size, + bool (*start_implicit_struct)(Visitor *v, void **obj, size_t size, Error **errp); /* May be NULL */ void (*end_implicit_struct)(Visitor *v); /* Must be set */ - void (*start_list)(Visitor *v, const char *name, GenericList **list, + bool (*start_list)(Visitor *v, const char *name, GenericList **list, Error **errp); /* Must be set */ GenericList *(*next_list)(Visitor *v, GenericList *element, Error **errp); diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index 4638863..4eae633 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -31,6 +31,27 @@ * visitor-impl.h. */ +/* All qapi types have a corresponding function with a signature + * roughly compatible with this: + * + * void visit_type_FOO(Visitor *v, void *obj, const char *name, Error **errp); + * + * where *@obj is itself a pointer or a scalar. The visit functions for + * built-in types are declared here, while the functions for qapi-defined + * struct, union, enum, and list types are generated (see qapi-visit.h). + * Input visitors may receive an uninitialized *@obj, and guarantee a + * safe value is assigned (a new object on success, or NULL on failure). + * Output visitors expect *@obj to be properly initialized on entry. + * + * Additionally, all qapi structs have a generated function compatible + * with this: + * + * void qapi_free_FOO(void *obj); + * + * which behaves like free(), even if @obj is NULL or was only partially + * allocated before encountering an error. + */ + /* This struct is layout-compatible with all other *List structs * created by the qapi generator. */ typedef struct GenericList @@ -62,11 +83,12 @@ typedef struct GenericList * Set *@errp on failure; for example, if the input stream does not * have a member @name or if the member is not an object. * - * FIXME: For input visitors, *@obj can be assigned here even if later - * visits will fail; this can lead to memory leaks if clients aren't - * careful. + * Returns true if *@obj was allocated; if that happens, and an error + * occurs any time before the matching visit_end_struct(), then the + * caller (usually a visit_type_FOO() function) knows to undo the + * allocation before returning control further. */ -void visit_start_struct(Visitor *v, const char *name, void **obj, +bool visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp); /** * Prepare for completing a struct visit. @@ -85,19 +107,15 @@ void visit_end_struct(Visitor *v); /** * Prepare to visit an implicit struct. - * Similar to visit_start_struct(), except that an implicit struct - * represents a subset of keys that are present at the same nesting level - * of a common object but as a separate qapi C struct, rather than a new - * object at a deeper nesting level. + * Similar to visit_start_struct(), including return semantics, except + * that an implicit struct represents a subset of keys that are present + * at the same nesting level of a common object but as a separate qapi + * C struct, rather than a new object at a deeper nesting level. * * @obj must not be NULL, since this function is only called when * visiting with qapi structs. - * - * FIXME: For input visitors, *@obj can be assigned here even if later - * visits will fail; this can lead to memory leaks if clients aren't - * careful. */ -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size, +bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp); /** * Complete an implicit struct visit started earlier. @@ -126,11 +144,12 @@ void visit_end_implicit_struct(Visitor *v); * in this case, visit_next_list() will not be needed, but * visit_end_list() is still mandatory. * - * FIXME: For input visitors, *@list can be assigned here even if - * later visits will fail; this can lead to memory leaks if clients - * aren't careful. + * Returns true if *@list was allocated; if that happens, and an error + * occurs any time before the matching visit_end_list(), then the + * caller (usually a visit_type_FOO() function) knows to undo the + * allocation before returning control further. */ -void visit_start_list(Visitor *v, const char *name, GenericList **list, +bool visit_start_list(Visitor *v, const char *name, GenericList **list, Error **errp); /** * Iterate over a GenericList during a list visit. diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index c5a7396..38d1e68 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -122,18 +122,20 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) } -static void +static bool opts_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { OptsVisitor *ov = to_ov(v); const QemuOpt *opt; + bool result = false; if (obj) { *obj = g_malloc0(size > 0 ? size : 1); + result = true; } if (ov->depth++ > 0) { - return; + return result; } ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, @@ -152,6 +154,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, ov->fake_id_opt->str = g_strdup(ov->opts_root->id); opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); } + return result; } @@ -210,7 +213,7 @@ lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) } -static void +static bool opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { OptsVisitor *ov = to_ov(v); @@ -223,8 +226,10 @@ opts_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) if (ov->repeated_opts) { ov->list_mode = LM_IN_PROGRESS; *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; + return false; } } diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 839049e..0990199 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -53,11 +53,12 @@ static void *qapi_dealloc_pop(QapiDeallocVisitor *qov) return value; } -static void qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, +static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void **obj, size_t unused, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, obj); + return false; } static void qapi_dealloc_end_struct(Visitor *v) @@ -69,13 +70,14 @@ static void qapi_dealloc_end_struct(Visitor *v) } } -static void qapi_dealloc_start_implicit_struct(Visitor *v, +static bool qapi_dealloc_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, obj); + return false; } static void qapi_dealloc_end_implicit_struct(Visitor *v) @@ -87,11 +89,12 @@ static void qapi_dealloc_end_implicit_struct(Visitor *v) } } -static void qapi_dealloc_start_list(Visitor *v, const char *name, +static bool qapi_dealloc_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); qapi_dealloc_push(qov, NULL); + return false; } static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *list, diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index f391a70..259e0cb 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -2,7 +2,7 @@ * Core Definitions for QAPI Visitor Classes * * Copyright IBM, Corp. 2011 - * Copyright (C) 2015 Red Hat, Inc. + * Copyright (C) 2015-2016 Red Hat, Inc. * * Authors: * Anthony Liguori <aliguori@us.ibm.com> @@ -26,14 +26,20 @@ static bool visit_is_output(Visitor *v) return v->type_enum == output_type_enum; } -void visit_start_struct(Visitor *v, const char *name, void **obj, +bool visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { + bool result; + assert(obj ? size : !size); if (obj && visit_is_output(v)) { assert(*obj); } - v->start_struct(v, name, obj, size, errp); + result = v->start_struct(v, name, obj, size, errp); + if (result) { + assert(obj && *obj); + } + return result; } void visit_check_struct(Visitor *v, Error **errp) @@ -48,16 +54,22 @@ void visit_end_struct(Visitor *v) v->end_struct(v); } -void visit_start_implicit_struct(Visitor *v, void **obj, size_t size, +bool visit_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp) { + bool result = false; + assert(obj && size); if (visit_is_output(v)) { assert(*obj); } if (v->start_implicit_struct) { - v->start_implicit_struct(v, obj, size, errp); + result = v->start_implicit_struct(v, obj, size, errp); } + if (result) { + assert(*obj); + } + return result; } void visit_end_implicit_struct(Visitor *v) @@ -67,10 +79,14 @@ void visit_end_implicit_struct(Visitor *v) } } -void visit_start_list(Visitor *v, const char *name, GenericList **list, +bool visit_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { - v->start_list(v, name, list, errp); + bool result = v->start_list(v, name, list, errp); + if (result) { + assert(list && *list); + } + return result; } GenericList *visit_next_list(Visitor *v, GenericList *list, Error **errp) @@ -229,6 +245,7 @@ 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: @@ -236,7 +253,11 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) assert(*obj); } */ - v->type_str(v, name, obj, errp); + v->type_str(v, name, obj, &err); + if (!visit_is_output(v) && err) { + *obj = NULL; + } + error_propagate(errp, err); } void visit_type_number(Visitor *v, const char *name, double *obj, @@ -248,11 +269,17 @@ 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); if (visit_is_output(v)) { assert(*obj); } - v->type_any(v, name, obj, errp); + v->type_any(v, name, obj, &err); + if (!visit_is_output(v) && err) { + *obj = NULL; + } + error_propagate(errp, err); } void visit_type_null(Visitor *v, const char *name, Error **errp) diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 82f9333..6b4ad68 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -130,7 +130,7 @@ static void qmp_input_pop(Visitor *v) qiv->nb_stack--; } -static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, +static bool qmp_input_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); @@ -140,30 +140,34 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); - return; + return false; } qmp_input_push(qiv, qobj, NULL, &err); if (err) { error_propagate(errp, err); - return; + return false; } if (obj) { *obj = g_malloc0(size); + return true; } + return false; } -static void qmp_input_start_implicit_struct(Visitor *v, void **obj, +static bool qmp_input_start_implicit_struct(Visitor *v, void **obj, size_t size, Error **errp) { if (obj) { *obj = g_malloc0(size); + return true; } + return false; } -static void qmp_input_start_list(Visitor *v, const char *name, +static bool qmp_input_start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { QmpInputVisitor *qiv = to_qiv(v); @@ -173,7 +177,7 @@ static void qmp_input_start_list(Visitor *v, const char *name, if (!qobj || qobject_type(qobj) != QTYPE_QLIST) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "list"); - return; + return false; } entry = qlist_first(qobject_to_qlist(qobj)); @@ -181,10 +185,12 @@ static void qmp_input_start_list(Visitor *v, const char *name, if (list) { if (entry) { *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; } } + return false; } static GenericList *qmp_input_next_list(Visitor *v, GenericList *list, diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 913f378..ce592d2 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -102,7 +102,7 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, } } -static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, +static bool qmp_output_start_struct(Visitor *v, const char *name, void **obj, size_t unused, Error **errp) { QmpOutputVisitor *qov = to_qov(v); @@ -110,6 +110,7 @@ static void qmp_output_start_struct(Visitor *v, const char *name, void **obj, qmp_output_add(qov, name, dict); qmp_output_push(qov, dict); + return false; } static void qmp_output_end_struct(Visitor *v) @@ -118,7 +119,7 @@ static void qmp_output_end_struct(Visitor *v) qmp_output_pop(qov, QTYPE_QDICT); } -static void qmp_output_start_list(Visitor *v, const char *name, +static bool qmp_output_start_list(Visitor *v, const char *name, GenericList **listp, Error **errp) { QmpOutputVisitor *qov = to_qov(v); @@ -126,6 +127,7 @@ static void qmp_output_start_list(Visitor *v, const char *name, qmp_output_add(qov, name, list); qmp_output_push(qov, list); + return false; } static GenericList *qmp_output_next_list(Visitor *v, GenericList *list, diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index 582a62a..0e4a07c 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -120,7 +120,7 @@ error: siv->ranges = NULL; } -static void +static bool start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { StringInputVisitor *siv = to_siv(v); @@ -132,7 +132,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) parse_str(siv, &err); if (err) { error_propagate(errp, err); - return; + return false; } siv->cur_range = g_list_first(siv->ranges); @@ -142,8 +142,10 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) siv->cur = r->begin; } *list = g_new0(GenericList, 1); + return true; } else { *list = NULL; + return false; } } diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 7209d80..2666802 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -263,7 +263,7 @@ static void print_type_number(Visitor *v, const char *name, double *obj, string_output_set(sov, g_strdup_printf("%f", *obj)); } -static void +static bool start_list(Visitor *v, const char *name, GenericList **list, Error **errp) { StringOutputVisitor *sov = to_sov(v); @@ -276,6 +276,7 @@ start_list(Visitor *v, const char *name, GenericList **list, Error **errp) if (*list && (*list)->next) { sov->list_mode = LM_STARTED; } + return false; } static GenericList * diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 6016734..eebb5f7 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -58,8 +58,10 @@ def gen_visit_implicit_struct(typ): static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp) { Error *err = NULL; + bool allocated; - visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err); + allocated = visit_start_implicit_struct(v, (void **)obj, + sizeof(%(c_type)s), &err); if (err) { goto out; } @@ -69,6 +71,10 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error * visit_type_%(c_type)s_fields(v, obj, &err); out_obj: visit_end_implicit_struct(v); + if (allocated && err) { + g_free(*obj); + *obj = NULL; + } out: error_propagate(errp, err); } @@ -116,18 +122,15 @@ 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) { Error *err = NULL; %(c_name)s *elt; + bool allocated; - visit_start_list(v, name, (GenericList **)obj, &err); + allocated = visit_start_list(v, name, (GenericList **)obj, &err); if (err) { goto out; } @@ -144,6 +147,10 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } visit_end_list(v); out: + if (allocated && err) { + qapi_free_%(c_name)s(*obj); + *obj = NULL; + } error_propagate(errp, err); } ''', @@ -174,8 +181,10 @@ def gen_visit_alternate(name, variants): void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; + bool allocated; - visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err); + allocated = visit_start_implicit_struct(v, (void **)obj, + sizeof(%(c_name)s), &err); if (err) { goto out; } @@ -204,11 +213,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error } out_obj: visit_end_implicit_struct(v); + if (allocated && 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 @@ -236,8 +249,10 @@ def gen_visit_object(name, base, members, variants): void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { Error *err = NULL; + bool allocated; - visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), &err); + allocated = visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), + &err); if (err) { goto out; } @@ -301,10 +316,15 @@ 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 (allocated && err) { + qapi_free_%(c_name)s(*obj); + *obj = NULL; + } out: error_propagate(errp, err); } -''') +''', + c_name=c_name(name)) return ret diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index bc8085d..d3466a4 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -227,14 +227,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); diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index 775ad39..4db35dd 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -181,10 +181,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, @@ -198,7 +195,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, @@ -212,7 +209,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, @@ -227,7 +224,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, @@ -241,7 +238,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, @@ -256,13 +253,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; Visitor *v; Error *err = NULL; @@ -270,7 +267,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 cd5e765..0eef7e0 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -733,18 +733,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,
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). Since input visitors have blind assignment semantics, we have to track the result of whether an assignment is made all the way down to each visitor callback implementation, to avoid making decisions based on potentially uninitialized storage. 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). Signed-off-by: Eric Blake <eblake@redhat.com> --- 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-impl.h | 6 ++--- include/qapi/visitor.h | 53 ++++++++++++++++++++++++++++-------------- qapi/opts-visitor.c | 11 ++++++--- qapi/qapi-dealloc-visitor.c | 9 ++++--- qapi/qapi-visit-core.c | 45 ++++++++++++++++++++++++++++------- qapi/qmp-input-visitor.c | 18 +++++++++----- qapi/qmp-output-visitor.c | 6 +++-- qapi/string-input-visitor.c | 6 +++-- qapi/string-output-visitor.c | 3 ++- scripts/qapi-visit.py | 40 +++++++++++++++++++++++-------- tests/test-qmp-commands.c | 13 +++++------ tests/test-qmp-input-strict.c | 19 +++++++-------- tests/test-qmp-input-visitor.c | 10 ++------ 13 files changed, 157 insertions(+), 82 deletions(-)