| Message ID | 20200311170417.13415-4-basil@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QGA - Win fixes | expand |
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"); >
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"); >>
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"); > >> >
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");
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(-)