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

Basil Salman posted 4 patches 7 years ago
Maintainers: Michael Roth <mdroth@linux.vnet.ibm.com>
There is a newer version of this series
[Qemu-devel] [PATCH v2 1/4] qga-win: prevent crash when executing guest-file-read with large count
Posted by Basil Salman 7 years ago
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


Re: [Qemu-devel] [PATCH v2 1/4] qga-win: prevent crash when executing guest-file-read with large count
Posted by Michael Roth 7 years ago
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
>