Re: [Qemu-devel] [PATCH v2 2/4] qga: fix send_response error handling

[Michael Roth]

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
>