Re: [PATCH v4 05/11] qmp: 'add_client' actually expects sockets

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Whether it is SPICE, VNC, D-Bus, or the socket chardev, they all
> actually expect a socket kind or will fail in different ways at runtime.
>
> Throw an error early if the given 'add_client' fd is not a socket, and
> close it to avoid leaks.
>
> This allows to replace the close() call with a more correct & portable
> closesocket() version.
>
> (this will allow importing sockets on Windows with a specialized command
> in the following patch, while keeping the remaining monitor associated
> sockets/add_client code & usage untouched)
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  qapi/misc.json     | 3 +++
>  monitor/qmp-cmds.c | 7 +++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 27ef5a2b20..f0217cfba0 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -14,6 +14,9 @@
>  # Allow client connections for VNC, Spice and socket based
>  # character devices to be passed in to QEMU via SCM_RIGHTS.
>  #
> +# If the FD associated with @fdname is not a socket, the command will fail and
> +# the FD will be closed.
> +#
>  # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
>  #            the name of a character device (eg. from -chardev id=XXXX)
>  #
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 859012aef4..9f7751beeb 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/sockets.h"
>  #include "monitor-internal.h"
>  #include "monitor/qdev.h"
>  #include "monitor/qmp-helpers.h"
> @@ -139,6 +140,12 @@ void qmp_add_client(const char *protocol, const char *fdname,
>          return;
>      }
>  
> +    if (!fd_is_socket(fd)) {
> +        error_setg(errp, "add_client expects a socket");

Let's mention the parameter name: "parameter @fdname must name a
socket".

> +        close(fd);
> +        return;
> +    }
> +
>      for (i = 0; i < ARRAY_SIZE(protocol_table); i++) {
>          if (!strcmp(protocol, protocol_table[i].name)) {
>              if (!protocol_table[i].add_client(fd, has_skipauth, skipauth,

Acked-by: Markus Armbruster <armbru@redhat.com>