[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read
From: |
Michael Roth |
Subject: |
Re: [PATCH v3 3/3] qga-win: prevent crash when executing guest-file-read with large count |
Date: |
Tue, 24 Mar 2020 11:49:20 -0500 |
User-agent: |
alot/0.7 |
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 <address@hidden>
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: address@hidden
> >
> >> Signed-off-by: Basil Salman <address@hidden>
> >> ---
> >> 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");
> >>
>