[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/4] qga: fix send_response error handling
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/4] qga: fix send_response error handling |
Date: |
Thu, 24 Jan 2019 11:18:46 -0600 |
User-agent: |
alot/0.7 |
Quoting Basil Salman (2019-01-13 04:05:29)
> Sometimes qemu-ga fails to send a response to client due to memory allocation
> issues due to a large response message, this can be experienced while trying
> to read large number of bytes using QMP command guest-file-read.
send_response has 2 areas that can fail:
1) When formatting the QDict *rsp from qmp_dispatch() into JSON via
qobject_to_json():
payload_qstr = qobject_to_json(QOBJECT(payload));
if (!payload_qstr) {
return -EINVAL;
}
But we can only reach that via qobject_to_json() calling qstring_new()
and that returning NULL. The qstring's initial size is independent of
the actual payload size. So I don't see how a large read would induce
this.
There is other code in qobject_to_json() -> to_json() that could maybe
hit an allocation failure once it start converting the payload to JSON,
but AFAICT that would cause a crash of qemu/qemu-ga once g_realloc()
finally fails to grow the qstring in qstring_append(), not an error
return.
So I don't think it's a bad idea to generate an error response like
you're doing in your patch for future cases, but I don't see how it
is reachable in the current code (even without the fix in patch 1).
Do you have a particular reproducer for this specific failure? Are
you sure it wasn't just the entire guest agent process crashing?
2) The other error you can get from send_response() is if there's
a problem writing things out to the actual communication channel,
in which case sending another error response likely won't help.
>
> Added a check to send an error response to qemu-ga client in such cases.
>
> Signed-off-by: Basil Salman <address@hidden>
> ---
> qga/main.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/qga/main.c b/qga/main.c
> index 87a0711c14..964275c40c 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -561,6 +561,8 @@ static void process_command(GAState *s, QDict *req)
> {
> QDict *rsp;
> int ret;
> + QDict *ersp;
> + Error *err = NULL;
>
> g_assert(req);
> g_debug("processing command");
> @@ -569,9 +571,20 @@ static void process_command(GAState *s, QDict *req)
> ret = send_response(s, rsp);
> if (ret < 0) {
> g_warning("error sending response: %s", strerror(-ret));
> + goto err;
> }
> qobject_unref(rsp);
> }
> + return;
> +err:
> + error_setg(&err, "Insufficient system resources exist to "
> + "complete the requested service");
> + ersp = qmp_error_response(err);
> + ret = send_response(s, ersp);
> + if (ret < 0) {
> + g_warning("error sending error response: %s", strerror(-ret));
> + }
> + qobject_unref(ersp);
> }
>
> /* handle requests/control events coming in over the channel */
> --
> 2.17.2
>