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

[Peter Maydell]

Ping! Any opinions, especially from qapi codegen people,
on the right thing to do here?

On Tue, 12 Apr 2022 at 20:04, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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