Re: [Xen-devel] [Xen-users] "xl restore" leaks a file descriptor?

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-08-12 at 18:12 +0100, Wei Liu wrote:
> On Wed, Aug 12, 2015 at 11:04:25AM +0100, Ian Campbell wrote:
> [...]
> > > > 
> > > > As Andy says I think we want restore_fd in the check, I can't see 
> > > > any
> > > > reason we wouldn't want to close the socket too.
> > > > 
> > > 
> > > Do you mean migrate_fd when you say "socket"?
> > 
> > In the migrate case we do "restore_fd = migrate_fd;", so yes, 
> > indirectly.
> > 
> > 
> > >  I tried that, but that led
> > > to failure because toolstack still needs to get controlling 
> > > information
> > > out of it (the "GO" message).
> > > 
> > > Maybe I close this too early.
> > 
> > Right.
> > 
> 
> I look at the code. Even if we should close that socket, it should not
> happen inside create_domain, because the caller (migrate_receive) needs
> that fd.
> 
> IMO create_domain should only close restore_fd if that fd is opened by
> itself.

That makes sense, yes. The close should probably have an associated comment
since this will be a bit subtle.

Perhaps rather than trying to repeat the conditions which lead to it being
opened we should just do:
    int restore_fd_to_close = -1;
    ...
    restore_fd_to_close = restore_fd = open(restore_file, O_RDONLY);
    ...
    if (restore_fd_to_close >= 0) {
        close(restore_fd_to_close);
        restore_fd_to_close = -1;
    }

Strictly speaking we ought to check the return of close too I suppose.

> Whether we should close send_fd and recv_fd in migrate_receive is
> another matter. I don't think we should. They are just stdin and stdout,
> not closing them wouldn't cause us any trouble.

The trouble they cause is holding kernel resources associated with the
socket, not to mention leaving a possible (perhaps unlikely) avenue of
attack from the network to a process which isn't expecting it...

Any we should be redirecting those to /dev/null as part of daemonising as a
matter of course and it looks like do_daemonize does that, so this is
already fine I think.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel