Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main()

From: Christian Schoenebeck <qemu_oss@crudebyte.com>
To: qemu-devel@nongnu.org
Cc: Greg Kurz <groug@kaod.org>, Alex Chen <alex.chen@huawei.com>,
	qemu-trivial@nongnu.org, zhang.zhanghailiang@huawei.com
Subject: Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main()
Date: Thu, 26 Nov 2020 19:44:24 +0100	[thread overview]
Message-ID: <13615133.E8RLdmFOkC@silver> (raw)
In-Reply-To: <20201126192719.7ea20846@bahia.lan>

On Donnerstag, 26. November 2020 19:27:19 CET Greg Kurz wrote:
> On Thu, 26 Nov 2020 18:52:39 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote:
> > > Hi Greg,
> > > 
> > > Thanks for your review.
> > > 
> > > On 2020/11/26 20:07, Greg Kurz wrote:
> > > > On Thu, 26 Nov 2020 10:16:24 +0000
> > > > 
> > > > Alex Chen <alex.chen@huawei.com> wrote:
> > > >> Only one of the options -s and -f can be used. When -f is used,
> > > >> the fd is created externally and does not need to be closed.
> > 
> > So somebody is really using the 9p proxy driver for something;
> > interesting.
> > 
> > > > The process running virtfs-proxy-helper has its own copy of
> > > > the fd inherited from its parent. And this fd will be closed
> > > > eventually when the process terminates.
> > > > 
> > > >> When -s is used, a new socket fd is created, and this socket fd
> > > >> needs to be closed at the end of main().
> > > > 
> > > > Same here, the new socket fd is closed when the process
> > > > terminates.
> > 
> > Does it? I haven't reviewed much of the 9p proxy code yet, however if
> > chroot() fails for instance, the fd would leak right now, wouldn't it?
> 
> This is done just at the end of main()... the leak won't last long.
> 
> > Or was your argument that it's the OS's job to free any file descriptor
> > automatically on process terminations in general?
> 
> That's exactly my point.
> 
> The only justification that'd deserve to be in the changelog of
> such a patch is something like "because this is good practice
> to rollback in case code moves to another function than main()".

Well, the actual motivation was rather a pragmatic one: to shut up a 
sanitizer's false positive, which I can understand.

Another option would be using a global variable for the fd instead of a 
temporary on stack. That should shut up the sanitizer as well and would not 
introduce change to the program flow.

I leave that up to Greg to decide whether or not to handle this. I'm 
Switzerland on this one.

Best regards,
Christian Schoenebeck