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

[Uri Lublin]

On 10/17/2016 01:08 PM, Frediano Ziglio wrote:
>>
>> Also replaced "continue" in while block with an empty
>> block (added curly braces).
>>
>
> I think you split this in 7/8.

Right, I'll remove it.

>
>> 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).

I think you are right, but also because of the dup2 that is a few lines
above the fcntl.

>
> 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...


right again.

I also noticed that if there are 2 (or more) QXL
devices, the same file is used for all, which is problematic

Thanks,
     Uri.