This fell through the cracks. My apologies.
Peter Maydell <peter.mayd...@linaro.org> writes:
> On Fri, 28 Feb 2020 at 09:26, Juan Quintela <quint...@redhat.com> wrote:
>>
>> It will be used later.
>>
>> Signed-off-by: Juan Quintela <quint...@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilb...@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.
