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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 13, 2015 at 09:39:36AM +0100, Ian Campbell wrote:
> 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.
> 

What would we do in case close fails?

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

Right.

Wei.

> Ian.

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