Re: [Qemu-devel] [PATCH v3] qga: Support enum names in guest-file-seek

[Eric Blake <eblake@redhat.com>]

On 3/20/20 8:49 AM, Peter Maydell wrote:
> On Tue, 9 Feb 2016 at 21:27, Eric Blake <eblake@redhat.com> wrote:
>>
>> Magic constants are a pain to use, especially when we run the
>> risk that our choice of '1' for QGA_SEEK_CUR might differ from
>> the host or guest's choice of SEEK_CUR.  Better is to use an
>> enum value, via a qapi alternate type for back-compatibility.
>>

> Hi; dragging up this patch from 2016 to say that Coverity
> has just noticed that there's some C undefined behaviour
> in it (CID 1421990):

Wow, took us a long time to find that!

> 
>> +/* Convert GuestFileWhence (either a raw integer or an enum value) into
>> + * the guest's SEEK_ constants.  */
>> +int ga_parse_whence(GuestFileWhence *whence, Error **errp)
>> +{
>> +    /* Exploit the fact that we picked values to match QGA_SEEK_*. */
>> +    if (whence->type == QTYPE_QSTRING) {
>> +        whence->type = QTYPE_QINT;
>> +        whence->u.value = whence->u.name;
> 
> Here whence->u.value and whence->u.name are two different
> fields in a union generated by QAPI:
> 
> typedef enum QGASeek {
>      QGA_SEEK_SET,
>      QGA_SEEK_CUR,
>      QGA_SEEK_END,
>      QGA_SEEK__MAX,
> } QGASeek;
> 
> struct GuestFileWhence {
>      QType type;
>      union { /* union tag is @type */
>          int64_t value;
>          QGASeek name;
>      } u;
> };
> 
> So u.value and u.name overlap in storage. The C standard
> says that this assignment is only valid if the overlap is
> exact and the two objects have qualified or unqualified
> versions of a compatible type. In this case the enum
> type is likely not the same size as an int64_t, and so
> we have undefined behaviour.

You are (well, Coverity is) absolutely right!  Patch coming up.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org