| Message ID | 1461801715-24307-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Our existing input visitors were not very consistent on errors > in a function taking 'TYPE **obj' (that is, start_struct(), > start_alternate(), next_list(), type_str(), and type_any()). > While all of them set '*obj' to allocated storage on success, > it was not obvious whether '*obj' was guaranteed safe on failure, > or whether it was left uninitialized. But a future patch wants > to guarantee that visit_type_FOO() does not leak a partially- > constructed obj back to the caller; it is easier to implement > this if we can reliably state that '*obj' is assigned on exit, > even on failures. Add assertions to enforce it. I had to read this several times, because by now I've forgotten that we're talking about input visitors only. Easy enough to avoid: ... that input visitors assign to *obj regardless of success or failure. Begs the question what is assigned to it on failure, though. > > The opts-visitor start_struct() doesn't set an error, but it > also was doing a weird check for 0 size; all callers pass in > non-zero size if obj is non-NULL. > > The testsuite has at least one spot where we no longer need > to pre-initialize a variable prior to a visit; valgrind confirms > that the test is still fine with the cleanup. > > A later patch will document the design constraint implemented > here. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v15: enhance commit message, hoist assertions from later in series > v14: no change > v13: no change > v12: new patch > --- > qapi/qapi-visit-core.c | 34 ++++++++++++++++++++++++++++++---- > qapi/opts-visitor.c | 3 ++- > qapi/qmp-input-visitor.c | 4 ++++ > qapi/string-input-visitor.c | 1 + > tests/test-qmp-input-strict.c | 2 +- > 5 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 3cd7edc..3a131ce 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -23,7 +23,13 @@ > void visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > - v->start_struct(v, name, obj, size, errp); > + Error *err = NULL; > + > + v->start_struct(v, name, obj, size, &err); > + if (obj && v->type == VISITOR_INPUT) { > + assert(err || *obj); > + } > + error_propagate(errp, err); > } The commit message claims you're adding assertions to enforce input visitors assign *obj even on failure. This assertion doesn't do that. It enforces "on success, *obj is non-null". Is that what you want? Or do you actually want something like "either err or *obj are non-null"? I.e. assert(!err != !*obj); > > void visit_end_struct(Visitor *v, Error **errp) > @@ -51,9 +57,15 @@ 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); > } > } > Hmm, you check the postcondition only when v implements start_alternate(). Shouldn't it hold regardless of v? If yes, then let's check it regardless of v: if (v->start_alternate) { v->start_alternate(v, name, obj, size, promote_int, &err); } if (v->type == VISITOR_INPUT) { assert(err || *obj); } error_propagate(errp, err); But that makes it pretty obvious that the postcondition won't hold when !v->start_alternate. May v->start_alternate() be null for an input visitor? According to visitor-impl.h, it may not. Okay. > @@ -188,7 +200,14 @@ 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) > { > - v->type_str(v, name, obj, errp); > + Error *err = NULL; > + > + assert(obj); > + 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, > @@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, double *obj, > > void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) > { > - v->type_any(v, name, obj, errp); > + Error *err = NULL; > + > + assert(obj); > + v->type_any(v, name, obj, &err); > + if (v->type == VISITOR_INPUT) { > + assert(err || *obj); > + } > + error_propagate(errp, err); > } > The commit message lists start_struct(), start_alternate(), next_list(), type_str(), and type_any(). You cover them except for next_list(). Why is that missing? > static void output_type_enum(Visitor *v, const char *name, int *obj, > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 66aeaed..4cb6436 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > const QemuOpt *opt; > > if (obj) { > - *obj = g_malloc0(size > 0 ? size : 1); > + *obj = g_malloc0(size); > } > if (ov->depth++ > 0) { > return; > @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) > > opt = lookup_scalar(ov, name, errp); > if (!opt) { > + *obj = NULL; > return; > } > *obj = g_strdup(opt->str ? opt->str : ""); > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 02d4233..77cce8b 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > QObject *qobj = qmp_input_get_object(qiv, name, true); > Error *err = NULL; > > + if (obj) { > + *obj = NULL; > + } > if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, > QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); > > if (!qstr) { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > return; > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index d604575..797973a 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, > if (siv->string) { > *obj = g_strdup(siv->string); > } else { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > } > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index d71727e..d5f80ec 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -263,7 +263,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, > static void test_validate_fail_alternate(TestInputVisitorData *data, > const void *unused) > { > - UserDefAlternate *tmp = NULL; > + UserDefAlternate *tmp; > Visitor *v; > Error *err = NULL;
On 04/28/2016 06:24 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Our existing input visitors were not very consistent on errors >> in a function taking 'TYPE **obj' (that is, start_struct(), >> start_alternate(), next_list(), type_str(), and type_any()). >> While all of them set '*obj' to allocated storage on success, >> it was not obvious whether '*obj' was guaranteed safe on failure, >> or whether it was left uninitialized. But a future patch wants >> to guarantee that visit_type_FOO() does not leak a partially- >> constructed obj back to the caller; it is easier to implement >> this if we can reliably state that '*obj' is assigned on exit, >> even on failures. Add assertions to enforce it. > > I had to read this several times, because by now I've forgotten that > we're talking about input visitors only. Easy enough to avoid: ... that > input visitors assign to *obj regardless of success or failure. > > Begs the question what is assigned to it on failure, though. Easy enough to improve the commit message - the intent is that these all set *obj to NULL on failure. >> + v->start_struct(v, name, obj, size, &err); >> + if (obj && v->type == VISITOR_INPUT) { >> + assert(err || *obj); >> + } >> + error_propagate(errp, err); >> } > > The commit message claims you're adding assertions to enforce input > visitors assign *obj even on failure. This assertion doesn't do that. > It enforces "on success, *obj is non-null". Is that what you want? Or > do you actually want something like "either err or *obj are non-null"? > I.e. > > assert(!err != !*obj); Hmm - that's an even tighter assertion. I'll run it through the testsuite, and if it passes, I'll use it. > > Hmm, you check the postcondition only when v implements > start_alternate(). Shouldn't it hold regardless of v? If yes, then > let's check it regardless of v: > > if (v->start_alternate) { > v->start_alternate(v, name, obj, size, promote_int, &err); > } > if (v->type == VISITOR_INPUT) { > assert(err || *obj); > } > error_propagate(errp, err); > > But that makes it pretty obvious that the postcondition won't hold when > !v->start_alternate. May v->start_alternate() be null for an input > visitor? According to visitor-impl.h, it may not. Okay. Doesn't hurt to make the check unconditional (to make sure no new input visitors forget to implement start_alternate). I'm also debating about adding an assertion (more likely in the doc patch, since it is less related to the theme of this one) that obj->type is sensible. > > The commit message lists start_struct(), start_alternate(), next_list(), > type_str(), and type_any(). You cover them except for next_list(). Why > is that missing? Because *obj can be NULL after next_list() if the list is empty. But there may still be a weaker assertion worth having: if err, then *obj must be NULL; and if *obj, then err must not be set (weaker in that for all the other functions touched, exactly one of the two conditions can result, but here, !err and !*obj is valid as a third condition). Depending on what else you find later in the series, I may just post a fixup for this patch.
On 04/28/2016 07:00 AM, Eric Blake wrote: >> The commit message lists start_struct(), start_alternate(), next_list(), >> type_str(), and type_any(). You cover them except for next_list(). Why >> is that missing? > > Because *obj can be NULL after next_list() if the list is empty. But > there may still be a weaker assertion worth having: if err, then *obj > must be NULL; and if *obj, then err must not be set (weaker in that for > all the other functions touched, exactly one of the two conditions can > result, but here, !err and !*obj is valid as a third condition). Actually, because visit_next_list() can't fail (we removed the errp argument earlier). When we finally move the list head allocation into visit_start_list later in the series (22/23), then we should add the (weaker) assert there. > > Depending on what else you find later in the series, I may just post a > fixup for this patch. Still my plan.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 3cd7edc..3a131ce 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -23,7 +23,13 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { - v->start_struct(v, name, obj, size, errp); + Error *err = NULL; + + v->start_struct(v, name, obj, size, &err); + if (obj && v->type == VISITOR_INPUT) { + assert(err || *obj); + } + error_propagate(errp, err); } void visit_end_struct(Visitor *v, Error **errp) @@ -51,9 +57,15 @@ 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); } } @@ -188,7 +200,14 @@ 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) { - v->type_str(v, name, obj, errp); + Error *err = NULL; + + assert(obj); + 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, @@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, double *obj, void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) { - v->type_any(v, name, obj, errp); + Error *err = NULL; + + assert(obj); + v->type_any(v, name, obj, &err); + if (v->type == VISITOR_INPUT) { + assert(err || *obj); + } + error_propagate(errp, err); } static void output_type_enum(Visitor *v, const char *name, int *obj, diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 66aeaed..4cb6436 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, const QemuOpt *opt; if (obj) { - *obj = g_malloc0(size > 0 ? size : 1); + *obj = g_malloc0(size); } if (ov->depth++ > 0) { return; @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) opt = lookup_scalar(ov, name, errp); if (!opt) { + *obj = NULL; return; } *obj = g_strdup(opt->str ? opt->str : ""); diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 02d4233..77cce8b 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, QObject *qobj = qmp_input_get_object(qiv, name, true); Error *err = NULL; + if (obj) { + *obj = NULL; + } if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); if (!qstr) { + *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); return; diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index d604575..797973a 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, if (siv->string) { *obj = g_strdup(siv->string); } else { + *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); } diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index d71727e..d5f80ec 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -263,7 +263,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, static void test_validate_fail_alternate(TestInputVisitorData *data, const void *unused) { - UserDefAlternate *tmp = NULL; + UserDefAlternate *tmp; Visitor *v; Error *err = NULL;
Our existing input visitors were not very consistent on errors in a function taking 'TYPE **obj' (that is, start_struct(), start_alternate(), next_list(), type_str(), and type_any()). While all of them set '*obj' to allocated storage on success, it was not obvious whether '*obj' was guaranteed safe on failure, or whether it was left uninitialized. But a future patch wants to guarantee that visit_type_FOO() does not leak a partially- constructed obj back to the caller; it is easier to implement this if we can reliably state that '*obj' is assigned on exit, even on failures. Add assertions to enforce it. The opts-visitor start_struct() doesn't set an error, but it also was doing a weird check for 0 size; all callers pass in non-zero size if obj is non-NULL. The testsuite has at least one spot where we no longer need to pre-initialize a variable prior to a visit; valgrind confirms that the test is still fine with the cleanup. A later patch will document the design constraint implemented here. Signed-off-by: Eric Blake <eblake@redhat.com> --- v15: enhance commit message, hoist assertions from later in series v14: no change v13: no change v12: new patch --- qapi/qapi-visit-core.c | 34 ++++++++++++++++++++++++++++++---- qapi/opts-visitor.c | 3 ++- qapi/qmp-input-visitor.c | 4 ++++ qapi/string-input-visitor.c | 1 + tests/test-qmp-input-strict.c | 2 +- 5 files changed, 38 insertions(+), 6 deletions(-)