From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [Xen-users] "xl restore" leaks a file descriptor? Date: Thu, 13 Aug 2015 09:39:36 +0100 Message-ID: <1439455176.23981.12.camel@citrix.com> References: <1438592915.30740.101.camel@citrix.com> <1439283311.9747.193.camel@citrix.com> <1439308093.9747.291.camel@citrix.com> <20150811170725.GU7460@zion.uk.xensource.com> <1439368873.9747.302.camel@citrix.com> <20150812094918.GY7460@zion.uk.xensource.com> <1439373865.9747.330.camel@citrix.com> <20150812171245.GA7460@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150812171245.GA7460@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-users@lists.xenproject.org, Andrew Armenia , Ian Jackson , xen-devel List-Id: xen-devel@lists.xenproject.org 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.