[Spice-devel] [spice-server PATCH 8/8] red-record-qxl: child_output_setup: check fcntl return value

[Frediano Ziglio]

> 
> Also replaced "continue" in while block with an empty
> block (added curly braces).
> 

I think you split this in 7/8.

> Found by coverity.
> 
> Signed-off-by: Uri Lublin <uril at redhat.com>
> ---
>  server/red-record-qxl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index adc487b..21fc35f 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -845,13 +845,18 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
>  static void child_output_setup(gpointer user_data)
>  {
>      int fd = GPOINTER_TO_INT(user_data);
> +    int r;
>  
>      while (dup2(fd, STDOUT_FILENO) < 0 && errno == EINTR) {
>      }
>      close(fd);
>  
>      // make sure file is not closed calling exec()
> -    fcntl(STDOUT_FILENO, F_SETFD, 0);
> +    while ((r = fcntl(STDOUT_FILENO, F_SETFD, 0)) < 0 && errno == EINTR) {
> +    }
> +    if (r < 0) {
> +        spice_error("fcntl F_SETFD failed (%d)\n", errno);
> +    }
>  }
>  
>  RedRecord *red_record_new(const char *filename)

I tried to understand better this.
First: fcntl(F_SETFD) cannot return EINTR so there's no reason to check
 (unless you check for kernel bugs).
This would change the code to

    if (fcntl(STDOUT_FILENO, F_SETFD, 0) < 0) {
        spice_error("fcntl F_SETFD failed (%d)\n", errno);
    }

Note however that probably this won't do what you are expecting.
This will put the message in the log and then spice-server will continue
and write the recording into a closed pipe so all fprintf internally will
call write which will return EPIPE.

Looking at the called function the file descriptor is the file descriptor
of "f" which is not created with O_CLOEXEC flag so the easier way to remove
this warning is just remove the fcntl line (which is doing nothing).

About O_CLOEXEC would be worth perhaps setting this flag after setting
up file/pipe (before the call to fwrite in red_record_new), something
like

    if (fcntl(fileno(f), F_SETFD, O_CLOEXEC) < 0) {
        spice_error("fcntl F_SETFD failed (%d)\n", errno);
    }

it's a bit racy but not racy would require using G_SPAWN_CLOEXEC_PIPES
in g_spawn_async_with_pipes which is supported since GLib 2.40 or manually
build the pipe with pipe2 and O_CLOEXEC and passing it to g_spawn_async_with_pipes
(and opening the record file with "w+e" instead of just "w+").
But I don't think that risking to leak this file descriptor is a big deal...

Frediano