Re: [PULL 03/15] multifd: Make no compression operations into its own structure

[Peter Maydell]

On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quintela@redhat.com> wrote:
>
> It will be used later.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>

Hi; Coverity thinks there might be a buffer overrun here.
It's probably wrong, but it's not completely obvious why
it can't happen, so an assert somewhere would help...
(This is CID 1487239.)

> +MultiFDCompression migrate_multifd_compression(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->parameters.multifd_compression;

This function returns an enum of type MultiFDCompression,
whose (autogenerated from QAPI) definition is:

typedef enum MultiFDCompression {
    MULTIFD_COMPRESSION_NONE,
    MULTIFD_COMPRESSION_ZLIB,
#if defined(CONFIG_ZSTD)
    MULTIFD_COMPRESSION_ZSTD,
#endif /* defined(CONFIG_ZSTD) */
    MULTIFD_COMPRESSION__MAX,
} MultiFDCompression;

> @@ -604,6 +745,7 @@ int multifd_save_setup(Error **errp)
>      multifd_send_state->pages = multifd_pages_init(page_count);
>      qemu_sem_init(&multifd_send_state->channels_ready, 0);
>      atomic_set(&multifd_send_state->exiting, 0);
> +    multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];

Here we take the result of the function and use it as an
array index into multifd_ops, whose definition is
 static MultiFDMethods *multifd_ops[MULTIFD_COMPRESSION__MAX] = { ... }

Coverity doesn't see any reason why the return value from
migrate_multifd_compression() can't be MULTIFD_COMPRESSION__MAX,
so it complains that if it is then we are going to index off the
end of the array.

An assert in migrate_multifd_compression() that the value being
returned is within the expected range would probably placate it.

Alternatively, if the qapi type codegen didn't put the __MAX
value as a possible value of the enum type then Coverity
and probably also the compiler wouldn't consider it to be
a possible value of this kind of variable. But that might
have other awkward side-effects.

thanks
-- PMM