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

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   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/address@hidden/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)