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

[Michael Roth]

Quoting Fam Zheng (2014-09-10 23:38:03)
> On Wed, 09/10 22:17, Eric Blake wrote:
> > On 09/10/2014 06:53 PM, Fam Zheng wrote:
> > > On Wed, 09/10 17:32, Paolo Bonzini wrote:
> > >> Il 10/09/2014 17:02, Fam Zheng ha scritto:
> > >>>> A bit hackish, but I don't have any better idea.
> > >>>>
> > >>>> Hmm... what about adding a new member to the visitors for "invalid 
> > >>>> enum"
> > >>>> value?  The dealloc visitor could override it to do nothing, while the
> > >>>> default could abort or set an error.  Would that work?
> > >>>
> > >>> The invalid state of enum still needs to be saved in the data.  It is 
> > >>> detected
> > >>> by the input visitor, but should be checked by other visitors (output, 
> > >>> dealloc)
> > >>> later.
> > >>
> > >> Yes, that's fine.  The only part where I'm not sure is the special
> > >> casing of the _MAX enum.
> > >>
> > > 
> > > Yes, it is abusing. Let's add an _INVALID = 0 enum which is much clearer.
> > 
> > If I understand correctly, you mean that for:
> > 
> > { 'enum': 'Foo', 'data': [ 'one', 'two' ] }
> > 
> > FOO_ONE would now be 1 instead of its current value of 0?
> > 
> > We just barely saw a case where Hu Tao's code was relying on the
> > implicit value 0 assigned to the first enum in the json file [1]
> > although I strongly argued that it should be nuked (and so it was fixed
> > in [2]).  So I could live with reserving 0 for internal use for flagging
> > parse errors (such as attempting to pass the string 'three' where a Foo
> > value is expected).
> > 
> > [1] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01691.html
> > [2] https://lists.gnu.org/archive/html/qemu-devel/2014-09/msg01938.html
> > 
> 
> A closer looking shows me a huge risk with _lookup table. We have for loops
> starting with index 0 all over the place to peek info _lookup arrays, but in
> this case it is _INVALID and shouldn't be looked at.

If we fix up the generator code to fill the 0 entry with a "_qapi_invalid"
string or something of the like then we shouldn't trigger any issues iterating
the lookup arrays.

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.

> 
> Maybe we have to put _INVALID at the end after _MAX.
> 
> Fam