qga: fix assert regression on guest-shutdown

Since commit 781f2b3d1e ("qga: process_event() simplification"),
send_response() is called unconditionally, but will assert when "rsp" is
NULL. This may

qga: fix assert regression on guest-shutdown

Since commit 781f2b3d1e ("qga: process_event() simplification"),
send_response() is called unconditionally, but will assert when "rsp" is
NULL. This may happen with QCO_NO_SUCCESS_RESP commands, such as
"guest-shutdown".

Fixes: 781f2b3d1e5ef389b44016a897fd55e7a780bf35
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga-win: Fix QGA VSS Provider service stop failure

On one hand "guest-fsfreeze-freeze" command, "COM+ System Application service" is
stopped, on the other hand "guest-fsfreeze-thaw" stops QGA VSS Pr

qga-win: Fix QGA VSS Provider service stop failure

On one hand "guest-fsfreeze-freeze" command, "COM+ System Application service" is
stopped, on the other hand "guest-fsfreeze-thaw" stops QGA VSS Provider service from
"COM+ Application Admin Catalog".
Invoking a series of freeze and thaw commands may result in QGA failing to stop
VSS Provider service as "COM+ System Application service" is stopped, which can
cause some delay in qga response.
In this commit StopService function was changed and VSS Provider service is now
stopped using Winsvc library API.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1549425

Signed-off-by: Basil Salman <bsalman@redhat.com>
Signed-off-by: Basil Salman <basil@daynix.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

error: Eliminate error_propagate() manually

When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. The prev

error: Eliminate error_propagate() manually

When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. The previous two commits did that for sufficiently simple
cases with Coccinelle. Do it for several more manually.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200707160613.848843-37-armbru@redhat.com>

show more ...

error: Eliminate error_propagate() with Coccinelle, part 1

When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right

error: Eliminate error_propagate() with Coccinelle, part 1

When all we do with an Error we receive into a local variable is
propagating to somewhere else, we can just as well receive it there
right away. Convert

if (!foo(..., &err)) {
...
error_propagate(errp, err);
...
return ...
}

to

if (!foo(..., errp)) {
...
...
return ...
}

where nothing else needs @err. Coccinelle script:

@rule1 forall@
identifier fun, err, errp, lbl;
expression list args, args2;
binary operator op;
constant c1, c2;
symbol false;
@@
if (
(
- fun(args, &err, args2)
+ fun(args, errp, args2)
|
- !fun(args, &err, args2)
+ !fun(args, errp, args2)
|
- fun(args, &err, args2) op c1
+ fun(args, errp, args2) op c1
)
)
{
... when != err
when != lbl:
when strict
- error_propagate(errp, err);
... when != err
(
return;
|
return c2;
|
return false;
)
}

@rule2 forall@
identifier fun, err, errp, lbl;
expression list args, args2;
expression var;
binary operator op;
constant c1, c2;
symbol false;
@@
- var = fun(args, &err, args2);
+ var = fun(args, errp, args2);
... when != err
if (
(
var
|
!var
|
var op c1
)
)
{
... when != err
when != lbl:
when strict
- error_propagate(errp, err);
... when != err
(
return;
|
return c2;
|
return false;
|
return var;
)
}

@depends on rule1 || rule2@
identifier err;
@@
- Error *err = NULL;
... when != err

Not exactly elegant, I'm afraid.

The "when != lbl:" is necessary to avoid transforming

if (fun(args, &err)) {
goto out
}
...
out:
error_propagate(errp, err);

even though other paths to label out still need the error_propagate().
For an actual example, see sclp_realize().

Without the "when strict", Coccinelle transforms vfio_msix_setup(),
incorrectly. I don't know what exactly "when strict" does, only that
it helps here.

The match of return is narrower than what I want, but I can't figure
out how to express "return where the operand doesn't use @err". For
an example where it's too narrow, see vfio_intx_enable().

Silently fails to convert hw/arm/armsse.c, because Coccinelle gets
confused by ARMSSE being used both as typedef and function-like macro
there. Converted manually.

Line breaks tidied up manually. One nested declaration of @local_err
deleted manually. Preexisting unwanted blank line dropped in
hw/riscv/sifive_e.c.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200707160613.848843-35-armbru@redhat.com>

show more ...

error: Avoid unnecessary error_propagate() after error_setg()

Replace

error_setg(&err, ...);
error_propagate(errp, err);

by

error_setg(errp, ...);

Related pattern:

if (...) {

error: Avoid unnecessary error_propagate() after error_setg()

Replace

error_setg(&err, ...);
error_propagate(errp, err);

by

error_setg(errp, ...);

Related pattern:

if (...) {
error_setg(&err, ...);
goto out;
}
...
out:
error_propagate(errp, err);
return;

When all paths to label out are that way, replace by

if (...) {
error_setg(errp, ...);
return;
}

and delete the label along with the error_propagate().

When we have at most one other path that actually needs to propagate,
and maybe one at the end that where propagation is unnecessary, e.g.

foo(..., &err);
if (err) {
goto out;
}
...
bar(..., &err);
out:
error_propagate(errp, err);
return;

move the error_propagate() to where it's needed, like

if (...) {
foo(..., &err);
error_propagate(errp, err);
return;
}
...
bar(..., errp);
return;

and transform the error_setg() as above.

In some places, the transformation results in obviously unnecessary
error_propagate(). The next few commits will eliminate them.

Bonus: the elimination of gotos will make later patches in this series
easier to review.

Candidates for conversion tracked down with this Coccinelle script:

@@
identifier err, errp;
expression list args;
@@
- error_setg(&err, args);
+ error_setg(errp, args);
... when != err
error_propagate(errp, err);

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200707160613.848843-34-armbru@redhat.com>

show more ...

qga: Plug unlikely memory leak in guest-set-memory-blocks

transfer_memory_block() leaks an Error object when reading file
/sys/devices/system/memory/memory<INDEX>/state fails with errno other
than E

qga: Plug unlikely memory leak in guest-set-memory-blocks

transfer_memory_block() leaks an Error object when reading file
/sys/devices/system/memory/memory<INDEX>/state fails with errno other
than ENOENT, and @sys2memblk is false, i.e. when the state file exists
but cannot be read (seems quite unlikely), and this is
guest-set-memory-blocks, not guest-get-memory-blocks.

Plug the leak.

Fixes: bd240fca42d5f072fb758a71720d9de9990ac553
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Message-Id: <20200630090351.1247703-9-armbru@redhat.com>

show more ...

qga: Fix qmp_guest_suspend_{disk, ram}() error handling

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter

qga: Fix qmp_guest_suspend_{disk, ram}() error handling

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>
Message-Id: <20200422130719.28225-15-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

show more ...

qga: Fix qmp_guest_get_memory_blocks() error handling

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL. Passing an argument of the
latter k

qga: Fix qmp_guest_get_memory_blocks() error handling

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.

qmp_guest_get_memory_blocks() passes &local_err to
transfer_memory_block() in a loop. If this fails in more than one
iteration, it can trip error_setv()'s assertion.

Fix it to break the loop.

Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200422130719.28225-14-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

show more ...

qga: Restrict guest-file-read count to 48 MB to avoid crashes

On [*] Daniel Berrangé commented:

The QEMU guest agent protocol is not sensible way to access huge
files inside the guest. It requi

qga: Restrict guest-file-read count to 48 MB to avoid crashes

On [*] Daniel Berrangé commented:

The QEMU guest agent protocol is not sensible way to access huge
files inside the guest. It requires the inefficient process of
reading the entire data into memory than duplicating it again in
base64 format, and then copying it again in the JSON serializer /
monitor code.

For arbitrary general purpose file access, especially for large
files, use a real file transfer program or use a network block
device, not the QEMU guest agent.

To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his
suggestion to put a low, hard limit on "count" in the guest agent
QAPI schema, and don't allow count to be larger than 48 MB.

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html

Fixes: CVE-2018-12617
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
*update schema documentation to indicate 48MB limit instead of 10MB
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga: Extract qmp_guest_file_read() to common commands.c

Extract the common code shared by both POSIX/Win32 implementations.

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

qga: Extract qmp_guest_file_read() to common commands.c

Extract the common code shared by both POSIX/Win32 implementations.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga: Extract guest_file_handle_find() to commands-common.h

As we are going to reuse this method, declare it in common
header.

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

qga: Extract guest_file_handle_find() to commands-common.h

As we are going to reuse this method, declare it in common
header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

Revert "prevent crash when executing guest-file-read with large count"

As noted by Daniel Berrangé in [*], the fix from commit 807e2b6fce
which replaced malloc() by try_malloc() is not enough, the p

Revert "prevent crash when executing guest-file-read with large count"

As noted by Daniel Berrangé in [*], the fix from commit 807e2b6fce
which replaced malloc() by try_malloc() is not enough, the process
can still run out of memory a few line later:

346 buf = g_try_malloc0(count + 1);
347 if (!buf) {
348 error_setg(errp,
349 "failed to allocate sufficient memory "
350 "to complete the requested service");
351 return NULL;
352 }
353 is_ok = ReadFile(fh, buf, count, &read_count, NULL);
354 if (!is_ok) {
355 error_setg_win32(errp, GetLastError(), "failed to read file");
356 slog("guest-file-read failed, handle %" PRId64, handle);
357 } else {
358 buf[read_count] = 0;
359 read_data = g_new0(GuestFileRead, 1);
^^^^^^

Instead we are going to put a low hard limit on 'count' in the next
commits. This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2.

[*] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03471.html

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga/commands-posix: fix use after free of local_err

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak

qga/commands-posix: fix use after free of local_err

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20200324153630.11882-7-vsementsov@virtuozzo.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>

show more ...

qemu-ga: document vsock-listen in the man page

Although qemu-ga has supported vsock since 2016 it was not documented on
the man page.

Also add the socket address representation to the qga --help ou

qemu-ga: document vsock-listen in the man page

Although qemu-ga has supported vsock since 2016 it was not documented on
the man page.

Also add the socket address representation to the qga --help output.

Fixes: 586ef5dee77180fc32e33bc08051600030630239
("qga: add vsock-listen method")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga: Fix undefined C behavior

The QAPI struct GuestFileWhence has a comment about how we are
exploiting equivalent values between two different integer types
shared in a union. But C says behavior i

qga: Fix undefined C behavior

The QAPI struct GuestFileWhence has a comment about how we are
exploiting equivalent values between two different integer types
shared in a union. But C says behavior is undefined on assignments to
overlapping storage when the two types are not the same width, and
indeed, 'int64_t value' and 'enum QGASeek name' are very likely to be
different in width. Utilize a temporary variable to fix things.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: 0b4b49387
Fixes: Coverity CID 1421990
Signed-off-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga-win: prevent crash when executing guest-file-read with large count

guest-file-read command is currently implemented to read from a
file handle count number of bytes. when executed with a very la

qga-win: prevent crash when executing guest-file-read with large count

guest-file-read command is currently implemented to read from a
file handle count number of bytes. when executed with a very large count number
qemu-ga crashes.
after some digging turns out that qemu-ga crashes after trying to allocate
a buffer large enough to save the data read in it, the buffer was allocated using
g_malloc0 which is not fail safe, and results a crash in case of failure.
g_malloc0 was replaced with g_try_malloc0() which returns NULL on failure,
A check was added for that case in order to prevent qemu-ga from crashing
and to send a response to the qemu-ga client accordingly.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054

Signed-off-by: Basil Salman <basil@daynix.com>
Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error

This patch handles the case where VSS Provider is already registered,
where in such case qga uninstalls the provider and registers it again.

qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error

This patch handles the case where VSS Provider is already registered,
where in such case qga uninstalls the provider and registers it again.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
Signed-off-by: Basil Salman <basil@daynix.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qga: Installer: Wait for installation to finish

Installation might fail if we don't wait for the provider
unregisteration process to finish.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
Signed

qga: Installer: Wait for installation to finish

Installation might fail if we don't wait for the provider
unregisteration process to finish.

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
Signed-off-by: Basil Salman <basil@daynix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

show more ...

qmp: constify QmpCommand and list

Since 0b69f6f72ce47a37a749b056b6d5ec64c61f11e8 "qapi: remove
qmp_unregister_command()", the command list can be declared const.

Signed-off-by: Marc-André Lureau <m

qmp: constify QmpCommand and list

Since 0b69f6f72ce47a37a749b056b6d5ec64c61f11e8 "qapi: remove
qmp_unregister_command()", the command list can be declared const.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Message-Id: <20200316171824.2319695-1-marcandre.lureau@redhat.com>
[Rebased]
Signed-off-by: Markus Armbruster <armbru@redhat.com>

show more ...