Re: [PULL 13/32] cpus: Fix configure_icount() error API violation

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <address@hidden> wrote:
>>
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>> call.
>>
>> configure_icount() is wrong that way.  Harmless, because its @errp is
>> always &error_abort or &error_fatal.
>>
>> Just as wrong (and just as harmless): when it fails, it can still
>> update global state.
>
> Hi; Coverity complains about this change (CID 1428754):
>>
>>  void configure_icount(QemuOpts *opts, Error **errp)
>>  {
>> -    const char *option;
>> +    const char *option = qemu_opt_get(opts, "shift");
>> +    bool sleep = qemu_opt_get_bool(opts, "sleep", true);
>> +    bool align = qemu_opt_get_bool(opts, "align", false);
>> +    long time_shift = -1;
>>      char *rem_str = NULL;
>>
>> -    option = qemu_opt_get(opts, "shift");
>> -    if (!option) {
>> -        if (qemu_opt_get(opts, "align") != NULL) {
>> -            error_setg(errp, "Please specify shift option when using 
>> align");
>> -        }
>> +    if (!option && qemu_opt_get(opts, "align")) {
>> +        error_setg(errp, "Please specify shift option when using align");
>>          return;
>
> Previously, if option was NULL we would always take this early
> exit. Now we only take the exit if option is NULL and the
> qemu_opt_get() returns true, so in some cases execution
> can continue through the function with a NULL option...
>
>>      }
>>
>> -    icount_sleep = qemu_opt_get_bool(opts, "sleep", true);
>> +    if (align && !sleep) {
>> +        error_setg(errp, "align=on and sleep=off are incompatible");
>> +        return;
>> +    }
>> +
>> +    if (strcmp(option, "auto") != 0) {
>
> ...but here we pass option to strcmp(), which is wrong if it
> can be NULL.

Right.  I'll post a fix.  Thank you!