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

[Peter Maydell]

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.

thanks
-- PMM