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

[Philippe Mathieu-Daudé]

On 4/22/20 5:17 PM, Markus Armbruster wrote:
> 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.

Oops I missed that, good catch!

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> > >            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)