[Qemu-devel] [PATCH v2 0/4] QGA - Win fixes

[Qemu-devel] [PATCH v2 0/4] QGA - Win fixes

[Basil Salman]

This patch series addresses serveral issues with qga-win please review
them and share your thoughts.

Basil Salman (3):
  qga-win: prevent crash when executing guest-file-read with large count
  qga: fix send_response error handling
  qga: Installer: Wait for installation to finish

Sameeh Jubran (1):
  qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error

 qga/commands-win32.c      |  8 +++++++-
 qga/installer/qemu-ga.wxs |  2 +-
 qga/main.c                | 13 +++++++++++++
 qga/vss-win32/install.cpp | 11 +++++++++++
 4 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/4] qga-win: prevent crash when executing guest-file-read with large count

[Basil Salman]

BZ: #1594054
guest-file-read command is currently implelmented 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.

Signed-off-by: Basil Salman <basil@daynix.com>
---
 qga/commands-win32.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 62e1b51dfe..4260faa573 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -345,7 +345,13 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
 
     fh = gfh->fh;
-    buf = g_malloc0(count+1);
+    buf = g_try_malloc0(count + 1);
+    if (!buf) {
+        error_setg(errp,
+                   "failed to allocate sufficient memory"
+                   "to complete the requested service");
+        return read_data;
+    }
     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
     if (!is_ok) {
         error_setg_win32(errp, GetLastError(), "failed to read file");
-- 
2.17.2

[Qemu-devel] [PATCH v2 2/4] qga: fix send_response error handling

[Basil Salman]

Sometimes qemu-ga fails to send a response to client due to memory allocation
issues due to a large response message, this can be experienced while trying
to read large number of bytes using QMP command guest-file-read.

Added a check to send an error response to qemu-ga client in such cases.

Signed-off-by: Basil Salman <basil@daynix.com>
---
 qga/main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/qga/main.c b/qga/main.c
index 87a0711c14..964275c40c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -561,6 +561,8 @@ static void process_command(GAState *s, QDict *req)
 {
     QDict *rsp;
     int ret;
+    QDict *ersp;
+    Error *err = NULL;
 
     g_assert(req);
     g_debug("processing command");
@@ -569,9 +571,20 @@ static void process_command(GAState *s, QDict *req)
         ret = send_response(s, rsp);
         if (ret < 0) {
             g_warning("error sending response: %s", strerror(-ret));
+            goto err;
         }
         qobject_unref(rsp);
     }
+    return;
+err:
+    error_setg(&err, "Insufficient system resources exist to "
+                      "complete the requested service");
+    ersp = qmp_error_response(err);
+    ret = send_response(s, ersp);
+    if (ret < 0) {
+        g_warning("error sending error response: %s", strerror(-ret));
+    }
+    qobject_unref(ersp);
 }
 
 /* handle requests/control events coming in over the channel */
-- 
2.17.2

[Qemu-devel] [PATCH v2 3/4] qga: Installer: Wait for installation to finish

[Basil Salman]

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>
---
 qga/installer/qemu-ga.wxs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/installer/qemu-ga.wxs b/qga/installer/qemu-ga.wxs
index 64bf90bd85..f6781752e6 100644
--- a/qga/installer/qemu-ga.wxs
+++ b/qga/installer/qemu-ga.wxs
@@ -81,7 +81,7 @@
               Arguments="-d --retry-path"
               >
             </ServiceInstall>
-            <ServiceControl Id="StartService" Start="install" Stop="both" Remove="uninstall" Name="QEMU-GA" Wait="no" />
+            <ServiceControl Id="StartService" Start="install" Stop="both" Remove="uninstall" Name="QEMU-GA" Wait="yes" />
           </Component>
           <?ifdef var.InstallVss?>
           <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
-- 
2.17.2

[Qemu-devel] [PATCH v2 4/4] qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error

[Basil Salman]

From: Sameeh Jubran <sjubran@redhat.com>

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>
---
 qga/vss-win32/install.cpp | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index 6713e58670..a456841360 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -443,6 +443,17 @@ STDAPI DllRegisterServer(void)
                                      VSS_PROV_SOFTWARE,
                                      const_cast<WCHAR*>(QGA_PROVIDER_VERSION),
                                      g_gProviderVersion);
+    if (hr == (long int) VSS_E_PROVIDER_ALREADY_REGISTERED) {
+        DllUnregisterServer();
+        hr = pVssAdmin->RegisterProvider(g_gProviderId, CLSID_QGAVSSProvider,
+                                         const_cast<WCHAR * >
+                                         (QGA_PROVIDER_LNAME),
+                                         VSS_PROV_SOFTWARE,
+                                         const_cast<WCHAR * >
+                                         (QGA_PROVIDER_VERSION),
+                                         g_gProviderVersion);
+    }
+
     if (FAILED(hr)) {
         errmsg_dialog(hr, "RegisterProvider failed");
     }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 1/4] qga-win: prevent crash when executing guest-file-read with large count

[Michael Roth]

Quoting Basil Salman (2019-01-13 04:05:28)
> BZ: #1594054
> guest-file-read command is currently implelmented to read from a

*implemented

> 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.
> 
> Signed-off-by: Basil Salman <basil@daynix.com>
> ---
>  qga/commands-win32.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 62e1b51dfe..4260faa573 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -345,7 +345,13 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>      }
> 
>      fh = gfh->fh;
> -    buf = g_malloc0(count+1);
> +    buf = g_try_malloc0(count + 1);
> +    if (!buf) {
> +        error_setg(errp,
> +                   "failed to allocate sufficient memory"
> +                   "to complete the requested service");
> +        return read_data;

return NULL might be a little clearer since that's what we do in the
preceeding checks

> +    }
>      is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>      if (!is_ok) {
>          error_setg_win32(errp, GetLastError(), "failed to read file");
> -- 
> 2.17.2
> 

Re: [Qemu-devel] [PATCH v2 2/4] qga: fix send_response error handling

[Michael Roth]

Quoting Basil Salman (2019-01-13 04:05:29)
> Sometimes qemu-ga fails to send a response to client due to memory allocation
> issues due to a large response message, this can be experienced while trying
> to read large number of bytes using QMP command guest-file-read.

send_response has 2 areas that can fail:

1) When formatting the QDict *rsp from qmp_dispatch() into JSON via
qobject_to_json():

    payload_qstr = qobject_to_json(QOBJECT(payload));
    if (!payload_qstr) {
        return -EINVAL;
    }

But we can only reach that via qobject_to_json() calling qstring_new()
and that returning NULL. The qstring's initial size is independent of
the actual payload size. So I don't see how a large read would induce
this.

There is other code in qobject_to_json() -> to_json() that could maybe
hit an allocation failure once it start converting the payload to JSON,
but AFAICT that would cause a crash of qemu/qemu-ga once g_realloc()
finally fails to grow the qstring in qstring_append(), not an error
return.

So I don't think it's a bad idea to generate an error response like
you're doing in your patch for future cases, but I don't see how it
is reachable in the current code (even without the fix in patch 1).

Do you have a particular reproducer for this specific failure? Are
you sure it wasn't just the entire guest agent process crashing?

2) The other error you can get from send_response() is if there's
a problem writing things out to the actual communication channel,
in which case sending another error response likely won't help.

> 
> Added a check to send an error response to qemu-ga client in such cases.
> 
> Signed-off-by: Basil Salman <basil@daynix.com>
> ---
>  qga/main.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 87a0711c14..964275c40c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -561,6 +561,8 @@ static void process_command(GAState *s, QDict *req)
>  {
>      QDict *rsp;
>      int ret;
> +    QDict *ersp;
> +    Error *err = NULL;
> 
>      g_assert(req);
>      g_debug("processing command");
> @@ -569,9 +571,20 @@ static void process_command(GAState *s, QDict *req)
>          ret = send_response(s, rsp);
>          if (ret < 0) {
>              g_warning("error sending response: %s", strerror(-ret));
> +            goto err;
>          }
>          qobject_unref(rsp);
>      }
> +    return;
> +err:
> +    error_setg(&err, "Insufficient system resources exist to "
> +                      "complete the requested service");
> +    ersp = qmp_error_response(err);
> +    ret = send_response(s, ersp);
> +    if (ret < 0) {
> +        g_warning("error sending error response: %s", strerror(-ret));
> +    }
> +    qobject_unref(ersp);
>  }
> 
>  /* handle requests/control events coming in over the channel */
> -- 
> 2.17.2
>