1. Patchew
  2. QEMU
  3. [PATCH v2 00/14] Miscellaneous error handling fixes
  4. View patch

[PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling

Markus Armbruster posted 14 patches 5 years, 9 months ago
Maintainers: Juan Quintela <quintela@redhat.com>, Kevin Wolf <kwolf@redhat.com>, John Snow <jsnow@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Richard Henderson <rth@twiddle.net>, Michael Roth <mdroth@linux.vnet.ibm.com>, "Michael S. Tsirkin" <mst@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, "Gonglei (Arei)" <arei.gonglei@huawei.com>, Peter Maydell <peter.maydell@linaro.org>, Max Reitz <mreitz@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Paul Durrant <paul@xen.org>, Anthony Perard <anthony.perard@citrix.com>, Jason Wang <jasowang@redhat.com>, Hailiang Zhang <zhang.zhanghailiang@huawei.com>, Paolo Bonzini <pbonzini@redhat.com>
[PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
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

qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
first to check_suspend_mode(), then to acquire_privilege(), then to
execute_async().  Continuing after errors here can only end in tears.
For instance, we risk tripping error_setv()'s assertion.

Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
Fixes: f54603b6aa765514b2519e74114a2f417759d727
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 9717a8d52d..5ba56327dd 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
 
     *mode = GUEST_SUSPEND_MODE_DISK;
     check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
     execute_async(do_suspend, mode, &local_err);
 
+out:
     if (local_err) {
         error_propagate(errp, local_err);
         g_free(mode);
@@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
 
     *mode = GUEST_SUSPEND_MODE_RAM;
     check_suspend_mode(*mode, &local_err);
+    if (local_err) {
+        goto out;
+    }
     acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
+    if (local_err) {
+        goto out;
+    }
     execute_async(do_suspend, mode, &local_err);
 
+out:
     if (local_err) {
         error_propagate(errp, local_err);
         g_free(mode);
-- 
2.21.1
Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago 
On 4/22/20 3:07 PM, Markus Armbruster 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
> 
> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
> first to check_suspend_mode(), then to acquire_privilege(), then to
> execute_async().  Continuing after errors here can only end in tears.
> For instance, we risk tripping error_setv()'s assertion.
> 
> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
> Fixes: f54603b6aa765514b2519e74114a2f417759d727
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 9717a8d52d..5ba56327dd 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_DISK;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {

https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is 
slightly different by removing the if() check.

>           error_propagate(errp, local_err);
>           g_free(mode);
> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>   
>       *mode = GUEST_SUSPEND_MODE_RAM;
>       check_suspend_mode(*mode, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>       execute_async(do_suspend, mode, &local_err);
>   
> +out:
>       if (local_err) {
>           error_propagate(errp, local_err);
>           g_free(mode);
>
Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
Posted by Markus Armbruster 5 years, 9 months ago 
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/22/20 3:07 PM, Markus Armbruster 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
>>
>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>> first to check_suspend_mode(), then to acquire_privilege(), then to
>> execute_async().  Continuing after errors here can only end in tears.
>> For instance, we risk tripping error_setv()'s assertion.
>>
>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qga/commands-win32.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 9717a8d52d..5ba56327dd 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>         *mode = GUEST_SUSPEND_MODE_DISK;
>>       check_suspend_mode(*mode, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       execute_async(do_suspend, mode, &local_err);
>>   +out:
>>       if (local_err) {
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
> slightly different by removing the if() check.

It frees @mode unconditionally (marked --> below) I believe that's
wrong.  execute_async() runs do_suspend() in a new thread, and passes it
@mode.  do_suspend() frees it.

>>           error_propagate(errp, local_err);
>>           g_free(mode);
>> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>>         *mode = GUEST_SUSPEND_MODE_RAM;
>>       check_suspend_mode(*mode, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>> +    if (local_err) {
>> +        goto out;
>> +    }
>>       execute_async(do_suspend, mode, &local_err);
>>   +out:
>>       if (local_err) {
>>           error_propagate(errp, local_err);
>>           g_free(mode);
>>

   diff --git a/qga/commands-win32.c b/qga/commands-win32.c
   index b49920e201..8b66098056 100644
   --- a/qga/commands-win32.c
   +++ b/qga/commands-win32.c
   @@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)

        *mode = GUEST_SUSPEND_MODE_DISK;
        check_suspend_mode(*mode, &local_err);
   +    if (local_err) {
   +        goto out;
   +    }
        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
   +    if (local_err) {
   +        goto out;
   +    }
        execute_async(do_suspend, mode, &local_err);

   -    if (local_err) {
   -        error_propagate(errp, local_err);
   -        g_free(mode);
   -    }
   +out:
   +    error_propagate(errp, local_err);
-->+    g_free(mode);
    }

    void qmp_guest_suspend_ram(Error **errp)
Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago 
On 4/22/20 5:17 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 4/22/20 3:07 PM, Markus Armbruster 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
>>>
>>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>>> first to check_suspend_mode(), then to acquire_privilege(), then to
>>> execute_async().  Continuing after errors here can only end in tears.
>>> For instance, we risk tripping error_setv()'s assertion.
>>>
>>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    qga/commands-win32.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>>> index 9717a8d52d..5ba56327dd 100644
>>> --- a/qga/commands-win32.c
>>> +++ b/qga/commands-win32.c
>>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>>          *mode = GUEST_SUSPEND_MODE_DISK;
>>>        check_suspend_mode(*mode, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        execute_async(do_suspend, mode, &local_err);
>>>    +out:
>>>        if (local_err) {
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
>> slightly different by removing the if() check.
> 
> It frees @mode unconditionally (marked --> below) I believe that's
> wrong.  execute_async() runs do_suspend() in a new thread, and passes it
> @mode.  do_suspend() frees it.

Oops I missed that, good catch!

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>>            error_propagate(errp, local_err);
>>>            g_free(mode);
>>> @@ -1338,9 +1345,16 @@ void qmp_guest_suspend_ram(Error **errp)
>>>          *mode = GUEST_SUSPEND_MODE_RAM;
>>>        check_suspend_mode(*mode, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>>        execute_async(do_suspend, mode, &local_err);
>>>    +out:
>>>        if (local_err) {
>>>            error_propagate(errp, local_err);
>>>            g_free(mode);
>>>
> 
>     diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>     index b49920e201..8b66098056 100644
>     --- a/qga/commands-win32.c
>     +++ b/qga/commands-win32.c
>     @@ -1341,13 +1341,18 @@ void qmp_guest_suspend_disk(Error **errp)
> 
>          *mode = GUEST_SUSPEND_MODE_DISK;
>          check_suspend_mode(*mode, &local_err);
>     +    if (local_err) {
>     +        goto out;
>     +    }
>          acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>     +    if (local_err) {
>     +        goto out;
>     +    }
>          execute_async(do_suspend, mode, &local_err);
> 
>     -    if (local_err) {
>     -        error_propagate(errp, local_err);
>     -        g_free(mode);
>     -    }
>     +out:
>     +    error_propagate(errp, local_err);
> -->+    g_free(mode);
>      }
> 
>      void qmp_guest_suspend_ram(Error **errp)
>
Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
Posted by Markus Armbruster 5 years, 9 months ago 
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/22/20 5:17 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 4/22/20 3:07 PM, Markus Armbruster 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
>>>>
>>>> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
>>>> first to check_suspend_mode(), then to acquire_privilege(), then to
>>>> execute_async().  Continuing after errors here can only end in tears.
>>>> For instance, we risk tripping error_setv()'s assertion.
>>>>
>>>> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
>>>> Fixes: f54603b6aa765514b2519e74114a2f417759d727
>>>> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>    qga/commands-win32.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>>>> index 9717a8d52d..5ba56327dd 100644
>>>> --- a/qga/commands-win32.c
>>>> +++ b/qga/commands-win32.c
>>>> @@ -1322,9 +1322,16 @@ void qmp_guest_suspend_disk(Error **errp)
>>>>          *mode = GUEST_SUSPEND_MODE_DISK;
>>>>        check_suspend_mode(*mode, &local_err);
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>>        acquire_privilege(SE_SHUTDOWN_NAME, &local_err);
>>>> +    if (local_err) {
>>>> +        goto out;
>>>> +    }
>>>>        execute_async(do_suspend, mode, &local_err);
>>>>    +out:
>>>>        if (local_err) {
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg695647.html is
>>> slightly different by removing the if() check.
>>
>> It frees @mode unconditionally (marked --> below) I believe that's
>> wrong.  execute_async() runs do_suspend() in a new thread, and passes it
>> @mode.  do_suspend() frees it.
>
> Oops I missed that, good catch!
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

I wasn't aware of (or totally forgot about) your patch, or else I'd have
fixed it instead of redoing it.  My apologies!
Re: [PATCH v2 14/14] qga: Fix qmp_guest_suspend_{disk, ram}() error handling
Posted by Eric Blake 5 years, 9 months ago 
On 4/22/20 8:07 AM, Markus Armbruster 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
> 
> qmp_guest_suspend_disk() and qmp_guest_suspend_ram() pass @local_err
> first to check_suspend_mode(), then to acquire_privilege(), then to
> execute_async().  Continuing after errors here can only end in tears.
> For instance, we risk tripping error_setv()'s assertion.
> 
> Fixes: aa59637ea1c6a4c83430933f9c44c43e6c3f1b69
> Fixes: f54603b6aa765514b2519e74114a2f417759d727
> Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qga/commands-win32.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.