1. Patchew
  2. QEMU
  3. [PULL 00/32] Miscellaneous patches for 2020-04-29
  4. View patch

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

Markus Armbruster posted 32 patches 5 years, 9 months ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Peter Maydell <peter.maydell@linaro.org>, Hailiang Zhang <zhang.zhanghailiang@huawei.com>, BALATON Zoltan <balaton@eik.bme.hu>, Michael Roth <mdroth@linux.vnet.ibm.com>, Gerd Hoffmann <kraxel@redhat.com>, "Gonglei (Arei)" <arei.gonglei@huawei.com>, Stefan Hajnoczi <stefanha@redhat.com>, Markus Armbruster <armbru@redhat.com>, Juan Quintela <quintela@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Wen Congyang <wencongyang2@huawei.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Fam Zheng <fam@euphon.net>, Max Reitz <mreitz@redhat.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Stefano Stabellini <sstabellini@kernel.org>, Kevin Wolf <kwolf@redhat.com>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, Richard Henderson <rth@twiddle.net>, Aurelien Jarno <aurelien@aurel32.net>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, John Snow <jsnow@redhat.com>, Bandan Das <bsd@redhat.com>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Xie Changlong <xiechanglong.d@gmail.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Anthony Perard <anthony.perard@citrix.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Alistair Francis <Alistair.Francis@wdc.com>, Jeff Cody <codyprime@gmail.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Alexander Bulekov <alxndr@bu.edu>, Thomas Huth <thuth@redhat.com>, Paul Durrant <paul@xen.org>, Laurent Vivier <lvivier@redhat.com>, Corey Minyard <cminyard@mvista.com>, Jason Wang <jasowang@redhat.com>
[PULL 13/32] cpus: Fix configure_icount() error API violation
Posted by Markus Armbruster 5 years, 9 months ago 
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.

Fix all that.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200422130719.28225-4-armbru@redhat.com>
---
 cpus.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/cpus.c b/cpus.c
index ef441bdf62..1b542b37f9 100644
--- a/cpus.c
+++ b/cpus.c
@@ -797,40 +797,49 @@ void cpu_ticks_init(void)
 
 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;
     }
 
-    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) {
+        errno = 0;
+        time_shift = strtol(option, &rem_str, 0);
+        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
+            error_setg(errp, "icount: Invalid shift value");
+            return;
+        }
+    } else if (icount_align_option) {
+        error_setg(errp, "shift=auto and align=on are incompatible");
+        return;
+    } else if (!icount_sleep) {
+        error_setg(errp, "shift=auto and sleep=off are incompatible");
+        return;
+    }
+
+    icount_sleep = sleep;
     if (icount_sleep) {
         timers_state.icount_warp_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
                                          icount_timer_cb, NULL);
     }
 
-    icount_align_option = qemu_opt_get_bool(opts, "align", false);
+    icount_align_option = align;
 
-    if (icount_align_option && !icount_sleep) {
-        error_setg(errp, "align=on and sleep=off are incompatible");
-    }
-    if (strcmp(option, "auto") != 0) {
-        errno = 0;
-        timers_state.icount_time_shift = strtol(option, &rem_str, 0);
-        if (errno != 0 || *rem_str != '\0' || !strlen(option)) {
-            error_setg(errp, "icount: Invalid shift value");
-        }
+    if (time_shift >= 0) {
+        timers_state.icount_time_shift = time_shift;
         use_icount = 1;
         return;
-    } else if (icount_align_option) {
-        error_setg(errp, "shift=auto and align=on are incompatible");
-    } else if (!icount_sleep) {
-        error_setg(errp, "shift=auto and sleep=off are incompatible");
     }
 
     use_icount = 2;
-- 
2.21.1
Re: [PULL 13/32] cpus: Fix configure_icount() error API violation
Posted by Peter Maydell 5 years, 9 months ago 
On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <armbru@redhat.com> 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
Re: [PULL 13/32] cpus: Fix configure_icount() error API violation
Posted by Markus Armbruster 5 years, 9 months ago 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 29 Apr 2020 at 08:34, Markus Armbruster <armbru@redhat.com> 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!

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.