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

[Markus Armbruster]

This fell through the cracks.  My apologies.

Peter Maydell <peter.maydell@linaro.org> writes:

> 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,

It is; details below.

>                      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;
>

Generated from

    { 'enum': 'MultiFDCompression',
      'data': [ 'none', 'zlib',
                { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }

>> @@ -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.

Yes.

migrate_multifd_compression() returns
current_migration->parameters.multifd_compression.

.multifd_compression is zero-initialized to MULTIFD_COMPRESSION_NONE,
and modified only by qmp_migrate_set_parameters().

qmp_migrate_set_parameters() can be called on behalf of QMP command
migrate-set-parameters, and on behalf of HMP command
migrate_set_parameter.  In either case, the value is the result of
parsing a string with qapi_enum_parse(), via visit_type_enum() and
visit_type_MultiFDCompression() with an input visitor (qobject for QMP,
string for HMP).  Never assigns the enum's __MAX.

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

Yes.

> 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.

Yes.  Coding the __MAX as a member of the enum is easy to write and easy
to understand, but gets in the way in places.

We could do something like

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

    #define MULTIFD_COMPRESSION__MAX ???

where ??? is 3 if defined(CONFIG_ZSTD), else 2.  The more conditionals,
the more awkward this gets.

The alternative is holes in the enum, like this:

    typedef enum MultiFDCompression {
        MULTIFD_COMPRESSION_NONE = 0,
        MULTIFD_COMPRESSION_ZLIB = 1,
    #if defined(CONFIG_ZSTD)
        MULTIFD_COMPRESSION_ZSTD = 2,
    #endif /* defined(CONFIG_ZSTD) */
    } MultiFDCompression;

    #define MULTIFD_COMPRESSION__MAX 3

Also puts holes into the lookup tables.

We'd need to review code to make sure we're not breaking "no holes"
assumptions.

Changing the __MAX from enum member to macro could conceivably break
something, too.  

The quick fix is an assertion.