[PATCH v3 0/3] QGA - Win fixes

[PATCH v3 0/3] QGA - Win fixes

[Basil Salman]

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

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

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

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

-- 
2.17.2

[PATCH v3 1/3] 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

[PATCH v3 2/3] 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

[PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count

[Basil Salman]

BZ: #1594054
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.

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 9c744d6405..b49920e201 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -343,7 +343,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 NULL;
+    }
     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: [PATCH v3 1/3] qga: Installer: Wait for installation to finish

[Philippe Mathieu-Daudé]

Hi Basil,

On 3/11/20 6:04 PM, Basil Salman wrote:
> 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" />

As 'yes' is the default, can you simply remove 'Wait="no"'?

>             </Component>
>             <?ifdef var.InstallVss?>
>             <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
> 

Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count

[Philippe Mathieu-Daudé]

On 3/11/20 6:04 PM, Basil Salman wrote:
> BZ: #1594054

^ This is not very helpful as it... (think to ppl with no knowledge of 
'BZ', what to do with this number). Instead ...

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

... add here (see 
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message):

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

Also add:

Cc: qemu-stable@nongnu.org

> 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 9c744d6405..b49920e201 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -343,7 +343,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 NULL;
> +    }

Can you fix the equivalent problem in qga/commands-posix.c too please?

Also use "PATCH-for-5.0" in the patch subject so we don't miss it for 
the next release.

Thanks!

Phil.

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

Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count

[Philippe Mathieu-Daudé]

On 3/24/20 2:20 PM, Philippe Mathieu-Daudé wrote:
> On 3/11/20 6:04 PM, Basil Salman wrote:
>> BZ: #1594054
> 
> ^ This is not very helpful as it... (think to ppl with no knowledge of 
> 'BZ', what to do with this number). Instead ...
> 
>> 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.
>>
> 
> ... add here (see 
> https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message): 
> 
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054

And per the BZ info, please also credit the reporter:

Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>

> 
> Also add:
> 
> Cc: qemu-stable@nongnu.org
> 
>> 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 9c744d6405..b49920e201 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -343,7 +343,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 NULL;
>> +    }
> 
> Can you fix the equivalent problem in qga/commands-posix.c too please?
> 
> Also use "PATCH-for-5.0" in the patch subject so we don't miss it for 
> the next release.
> 
> Thanks!
> 
> Phil.
> 
>>       is_ok = ReadFile(fh, buf, count, &read_count, NULL);
>>       if (!is_ok) {
>>           error_setg_win32(errp, GetLastError(), "failed to read file");
>>

Re: [PATCH v3 1/3] qga: Installer: Wait for installation to finish

[Marc-André Lureau]

Hi

On Tue, Mar 24, 2020 at 2:13 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Basil,
>
> On 3/11/20 6:04 PM, Basil Salman wrote:
> > 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" />
>
> As 'yes' is the default, can you simply remove 'Wait="no"'?

It looks like wixl doesn't follow the WiX default value though.

tools/wixl/msi.vala:
574                 (Wait != null && !rec.set_int (5, Wait ? 1 : 0)) ||

>
> >             </Component>
> >             <?ifdef var.InstallVss?>
> >             <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
> >
>
>


-- 
Marc-André Lureau

Re: [PATCH v3 1/3] qga: Installer: Wait for installation to finish

[Philippe Mathieu-Daudé]

On 3/24/20 3:00 PM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Mar 24, 2020 at 2:13 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> Hi Basil,
>>
>> On 3/11/20 6:04 PM, Basil Salman wrote:
>>> 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" />
>>
>> As 'yes' is the default, can you simply remove 'Wait="no"'?
> 
> It looks like wixl doesn't follow the WiX default value though.
> 
> tools/wixl/msi.vala:
> 574                 (Wait != null && !rec.set_int (5, Wait ? 1 : 0)) ||

I trusted the documentation...
https://wixtoolset.org/documentation/manual/v3/xsd/wix/servicecontrol.html

Thanks for looking at the implementation.

So for this patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>
>>>              </Component>
>>>              <?ifdef var.InstallVss?>
>>>              <Component Id="qga_vss_dll" Guid="{CB19C453-FABB-4BB1-ABAB-6B74F687BFBB}">
>>>
>>
>>
> 
> 

Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2020-03-24 08:37:05)
> On 3/24/20 2:20 PM, Philippe Mathieu-Daudé wrote:
> > On 3/11/20 6:04 PM, Basil Salman wrote:
> >> BZ: #1594054
> > 
> > ^ This is not very helpful as it... (think to ppl with no knowledge of 
> > 'BZ', what to do with this number). Instead ...
> > 
> >> 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.
> >>
> > 
> > ... add here (see 
> > https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message): 
> > 
> > 
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
> 
> And per the BZ info, please also credit the reporter:
> 
> Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>

Since I had these queued for a pull already I went ahead and rolled your
suggestions (minus the posix-side fix) into this patch.

A seperate follow-up patch address the posix counterpart would still be
appreciated though.

> 
> > 
> > Also add:
> > 
> > Cc: qemu-stable@nongnu.org
> > 
> >> 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 9c744d6405..b49920e201 100644
> >> --- a/qga/commands-win32.c
> >> +++ b/qga/commands-win32.c
> >> @@ -343,7 +343,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 NULL;
> >> +    }
> > 
> > Can you fix the equivalent problem in qga/commands-posix.c too please?
> > 
> > Also use "PATCH-for-5.0" in the patch subject so we don't miss it for 
> > the next release.
> > 
> > Thanks!
> > 
> > Phil.
> > 
> >>       is_ok = ReadFile(fh, buf, count, &read_count, NULL);
> >>       if (!is_ok) {
> >>           error_setg_win32(errp, GetLastError(), "failed to read file");
> >>
>