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

From: Peter Maydell <peter.maydell@linaro.org>
To: Juan Quintela <quintela@redhat.com>
Cc: qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	 Markus Armbruster <armbru@redhat.com>,
	Michael Roth <michael.roth@amd.com>
Subject: Re: [PULL 03/15] multifd: Make no compression operations into its own structure
Date: Tue, 19 Jul 2022 15:06:09 +0100	[thread overview]
Message-ID: <CAFEAcA9C6f95fEgowODv+xQJd8pNDTb2W8LBB5+swRGO+a1hdg@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA9zp1QoKObY_=L8xPmuK7xEvcM+9hohW0fV+KE3fVBAOw@mail.gmail.com>

Ping^2! I remain unsure how we should resolve this Coverity complaint
and would like opinions from QAPI codegen people.

thanks
-- PMM

On Fri, 13 May 2022 at 18:56, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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