[Spice-devel] [PATCH server v2 05/13] red-channel: send marshaller message fd

[Marc-André Lureau]

Hi

----- Original Message -----
> > 
> > Hi
> > 
> > On Fri, Jan 15, 2016 at 11:44 AM, Frediano Ziglio <fziglio at redhat.com>
> > wrote:
> > >>
> > >> From: Marc-André Lureau <mlureau at redhat.com>
> > >>
> > >> Send the fd associated to the last message sent.
> > >>
> > >> Even if the fd is invalid, the sendfd msg is appended to the protocol,
> > >> for 2 reasons:
> > >> - trying to send an invalid fd does not have to close the connection (it
> > >>   would with an invalid fd)
> > >> - even if the fd is invalid, the protocol expects an extra byte for the
> > >>   ancillary data
> > >>
> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> > >> ---
> > >>  server/red-channel.c | 16 ++++++++++++++++
> > >>  1 file changed, 16 insertions(+)
> > >>
> > >> diff --git a/server/red-channel.c b/server/red-channel.c
> > >> index 306c87d..b33c91d 100644
> > >> --- a/server/red-channel.c
> > >> +++ b/server/red-channel.c
> > >> @@ -608,8 +608,24 @@ static inline void
> > >> red_channel_client_release_sent_item(RedChannelClient *rcc)
> > >>  static void red_channel_peer_on_out_msg_done(void *opaque)
> > >>  {
> > >>      RedChannelClient *rcc = (RedChannelClient *)opaque;
> > >> +    int fd = spice_marshaller_get_fd(rcc->send_data.marshaller);
> > >>
> > >>      rcc->send_data.size = 0;
> > >> +
> > >> +    if (fd != -1) {
> > >> +        if (fcntl(fd, F_GETFD) == -1) {
> > >
> > > Is not clear what's the reason for this test.
> > > Why should fail?
> > 
> > It should not fail, but if the caller has set an invalid fd (or it
> > became invalid), I think it is better to deal with it and not
> > disconnect the client. (trying to send an invalid fd over sendmsg
> > closes the connection)
> > 
> 
> The file descriptor is saved with a dup. If the handle became invalid
> it means some internal corruption happened. In this case I would even
> abort the program.
> 

Ok, I don't think I have good enough reasons to keep this either. Let's remove the check.

> > >
> > >> +            close(fd);
> > >> +            fd = -1;
> > >> +        }
> > >> +
> > >> +        if (reds_stream_send_msgfd(rcc->stream, fd) < 0) {
> > >> +            perror("sendfd");
> > >> +            red_channel_client_disconnect(rcc);
> > >> +            return;
> > >
> > > I think here you should close the file descriptor too to avoid leak.
> > >
> > 
> > agreed
> > 
> > >> +        }
> > >> +        close(fd);
> > >> +    }
> > >> +
> > >>      red_channel_client_release_sent_item(rcc);
> > >>      if (rcc->send_data.blocked) {
> > >>          rcc->send_data.blocked = FALSE;
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>