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

[Peter Maydell]

On Tue, 9 Feb 2016 at 21:27, Eric Blake <address@hidden> 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.
>
> With this,
>  {"command":"guest-file-seek", "arguments":{"handle":1,
>   "offset":0, "whence":"cur"}}
> becomes a synonym for the older
>  {"command":"guest-file-seek", "arguments":{"handle":1,
>   "offset":0, "whence":1}}
>
> Signed-off-by: Eric Blake <address@hidden>

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):

> +/* 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.

I guess to fix this we need to copy via a local variable
(with a comment so nobody helpfully optimizes the local
away again in future)...

> +    }
> +    switch (whence->u.value) {
> +    case QGA_SEEK_SET:
> +        return SEEK_SET;
> +    case QGA_SEEK_CUR:
> +        return SEEK_CUR;
> +    case QGA_SEEK_END:
> +        return SEEK_END;
> +    }
> +    error_setg(errp, "invalid whence code %"PRId64, whence->u.value);
> +    return -1;
> +}

thanks
-- PMM