Hi
On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <ebl...@redhat.com> wrote:
> Commit cee2dedb noticed that if you have a partial flat union
> (such as if an input parse failed due to a missing
> discriminator), calling the dealloc visitor could result in
> trying to dereference the NULL pointer. But the fix it proposed
> requires the use of a 'data' member in the union, which may or
> may not be the same size as other branches of the union
> (consider a 32-bit platform where one of the branches is an
> int64), so it feels fairly dirty. A better fix is to tweak all
> of the generated visit_type_implicit_FOO() functions to avoid
> dereferencing NULL in the first place, by not visiting the
> fields if the struct pointer itself is not present, at which
> point we no longer even need visit_start_union(). And no one
> was implementing visit_end_union() callbacks.
>
> While rewriting the code, use patterns that are closer to what
> is used elsewhere in the generated visitors, by using 'goto'
> to cleanup labels rather than putting followup code under 'if'
> conditions. The change keeps the contract that any successful
> use of visit_start_implicit_struct() will be paired with a
> matching visit_end_implicit_struct(), even if intermediate
> processing is skipped. We are safe in checking *obj alone, as
> as the contract of visit_start_implicit_struct() requires a
> non-NULL obj.
>
> As an example of the changes to generated code:
> |@@ -1331,10 +1331,16 @@ static void visit_type_implicit_Blockdev
> | Error *err = NULL;
> |
> | visit_start_implicit_struct(v, (void **)obj,
> sizeof(BlockdevOptionsArchipelago), &err);
> |- if (!err) {
> |- visit_type_BlockdevOptionsArchipelago_fields(v, obj, errp);
> |- visit_end_implicit_struct(v);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+ if (!*obj) {
> |+ goto out_obj;
> | }
> |+ visit_type_BlockdevOptionsArchipelago_fields(v, obj, &err);
> |+out_obj:
> |+ visit_end_implicit_struct(v);
> |+out:
> | error_propagate(errp, err);
> | }
> ...
> |@@ -1479,9 +1539,6 @@ void visit_type_BlockdevOptions(Visitor
> | if (err) {
> | goto out_obj;
> | }
> |- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
> |- goto out_obj;
> |- }
> | switch ((*obj)->driver) {
> | case BLOCKDEV_DRIVER_ARCHIPELAGO:
> | visit_type_implicit_BlockdevOptionsArchipelago(v,
> &(*obj)->u.archipelago, &err);
> |@@ -1570,11 +1627,6 @@ void visit_type_BlockdevOptions(Visitor
> | out_obj:
> | error_propagate(errp, err);
> | err = NULL;
> |- if (*obj) {
> |- visit_end_union(v, !!(*obj)->u.data, &err);
> |- }
> |- error_propagate(errp, err);
> |- err = NULL;
> | visit_end_struct(v, &err);
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
--
Marc-André Lureau
