* [PATCH v3 0/3] QGA - Win fixes @ 2020-03-11 17:04 Basil Salman 2020-03-11 17:04 ` [PATCH v3 1/3] qga: Installer: Wait for installation to finish Basil Salman ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Basil Salman @ 2020-03-11 17:04 UTC (permalink / raw) To: qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/3] qga: Installer: Wait for installation to finish 2020-03-11 17:04 [PATCH v3 0/3] QGA - Win fixes Basil Salman @ 2020-03-11 17:04 ` Basil Salman 2020-03-24 13:12 ` Philippe Mathieu-Daudé 2020-03-11 17:04 ` [PATCH v3 2/3] qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error Basil Salman 2020-03-11 17:04 ` [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count Basil Salman 2 siblings, 1 reply; 10+ messages in thread From: Basil Salman @ 2020-03-11 17:04 UTC (permalink / raw) To: qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] qga: Installer: Wait for installation to finish 2020-03-11 17:04 ` [PATCH v3 1/3] qga: Installer: Wait for installation to finish Basil Salman @ 2020-03-24 13:12 ` Philippe Mathieu-Daudé 2020-03-24 14:00 ` Marc-André Lureau 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-03-24 13:12 UTC (permalink / raw) To: Basil Salman, qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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}"> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] qga: Installer: Wait for installation to finish 2020-03-24 13:12 ` Philippe Mathieu-Daudé @ 2020-03-24 14:00 ` Marc-André Lureau 2020-03-24 14:03 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 10+ messages in thread From: Marc-André Lureau @ 2020-03-24 14:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Yan Vugenfirer, Basil Salman, QEMU, Michael Roth 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] qga: Installer: Wait for installation to finish 2020-03-24 14:00 ` Marc-André Lureau @ 2020-03-24 14:03 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-03-24 14:03 UTC (permalink / raw) To: Marc-André Lureau; +Cc: Yan Vugenfirer, Basil Salman, QEMU, Michael Roth 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}"> >>> >> >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error 2020-03-11 17:04 [PATCH v3 0/3] QGA - Win fixes Basil Salman 2020-03-11 17:04 ` [PATCH v3 1/3] qga: Installer: Wait for installation to finish Basil Salman @ 2020-03-11 17:04 ` Basil Salman 2020-03-11 17:04 ` [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count Basil Salman 2 siblings, 0 replies; 10+ messages in thread From: Basil Salman @ 2020-03-11 17:04 UTC (permalink / raw) To: qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count 2020-03-11 17:04 [PATCH v3 0/3] QGA - Win fixes Basil Salman 2020-03-11 17:04 ` [PATCH v3 1/3] qga: Installer: Wait for installation to finish Basil Salman 2020-03-11 17:04 ` [PATCH v3 2/3] qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error Basil Salman @ 2020-03-11 17:04 ` Basil Salman 2020-03-24 13:20 ` Philippe Mathieu-Daudé 2 siblings, 1 reply; 10+ messages in thread From: Basil Salman @ 2020-03-11 17:04 UTC (permalink / raw) To: qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count 2020-03-11 17:04 ` [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count Basil Salman @ 2020-03-24 13:20 ` Philippe Mathieu-Daudé 2020-03-24 13:37 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-03-24 13:20 UTC (permalink / raw) To: Basil Salman, qemu-devel, Michael Roth; +Cc: Yan Vugenfirer 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"); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count 2020-03-24 13:20 ` Philippe Mathieu-Daudé @ 2020-03-24 13:37 ` Philippe Mathieu-Daudé 2020-03-24 16:49 ` Michael Roth 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2020-03-24 13:37 UTC (permalink / raw) To: Basil Salman, qemu-devel, Michael Roth; +Cc: Yan Vugenfirer, Fakhri Zulkifli 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"); >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count 2020-03-24 13:37 ` Philippe Mathieu-Daudé @ 2020-03-24 16:49 ` Michael Roth 0 siblings, 0 replies; 10+ messages in thread From: Michael Roth @ 2020-03-24 16:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Basil Salman, qemu-devel Cc: Yan Vugenfirer, Fakhri Zulkifli 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"); > >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-24 17:28 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-11 17:04 [PATCH v3 0/3] QGA - Win fixes Basil Salman 2020-03-11 17:04 ` [PATCH v3 1/3] qga: Installer: Wait for installation to finish Basil Salman 2020-03-24 13:12 ` Philippe Mathieu-Daudé 2020-03-24 14:00 ` Marc-André Lureau 2020-03-24 14:03 ` Philippe Mathieu-Daudé 2020-03-11 17:04 ` [PATCH v3 2/3] qga-win: Handle VSS_E_PROVIDER_ALREADY_REGISTERED error Basil Salman 2020-03-11 17:04 ` [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count Basil Salman 2020-03-24 13:20 ` Philippe Mathieu-Daudé 2020-03-24 13:37 ` Philippe Mathieu-Daudé 2020-03-24 16:49 ` Michael Roth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.