Re: [Qemu-devel] [PATCH] qapi: Fix crash with enum dealloc when kind is invalid

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Paolo Bonzini (2014-09-11 09:35:58)
> Il 11/09/2014 16:26, Michael Roth ha scritto:
> > Also, the .kind field of a QAPI Union type is something we generate for use
> > by the generated visitor code. In the case of an unspecified discriminator
> > we generated the enum type for that field internally. In the case where it's
> > specified, we use an existing enum instead...
> > 
> > But nothing stops us from generating a new "shadow" enum in this case as well,
> > with the indexes/integer values of the corresponding strings shifted by one so
> > we can reserve the 0 index for _INVALID. I think we can reasonably expect that
> > nothing outside the generated code makes use of those integer values in this
> > special case, and don't have to change all enum types to make that work.
> 
> But how would users fill in structs if you have to use a different enum?

Argh, of course, we do still make direct use of these going in the other
direction. Those users would need to use the "shadow" enum values to make
it work, which is probably way too messy.

> 
> What about making adding visit_start_union/visit_end_union?
> visit_start_union can return false if the visit of the union has to be
> skipped.
> 
> The dealloc visitor can skip it if the data field is NULL; everything
> else can just use a default implementation which always returns true.

I forgot we had a void *data there as well. So we're basically relying
on .data != NULL implying that .kind has been properly initialized,
rather than needing to encode anything into .kind... nice.

I can imagine a case where we allocate memory for a set of union fields
(so .data != NULL) and then leave .kind uninitialized, which can still
lead to segfaults due to improper casts in the dealloc visitor, but I
don't really see a way around that. Even if we reserve .kind == 0 for
this purpose, it's still up to the user or visitor implementation to
0-initialize everything (though that's a bit easier to enforce).

So this seems like a good approach. I've ahead and hacked something up
which I'll send out shortly.

> 
> Paolo