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

[Greg Kurz <groug@kaod.org>]

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()".

> > IMO, it's best to explicitly release resources before the process
> > terminates, just as the variable 'rpath' is explicitly freed in main(),
> > so socket fd also needs to be explicitly closed here.
> > 
> > Looking forward to your reply.
> > 
> > > The only justification to merge such a change would be if
> > > the code was sitting in some other function, in which
> > > case we should indeed do proper rollback. But it is main()
> > > here, so this patch isn't needed.
> > > 
> > >> Reported-by: Euler Robot <euler.robot@huawei.com>
> > > 
> > > Can you provide a copy of the report in case I'm
> > > missing something ?
> > 
> > Our codecheck tool reports a resource leak here, which is relatively simple,
> > like the one below, I did not attach it.
> > 
> > ---------------------
> > "Resource leak: sock"
> > ---------------------
> 
> Yeah, not very helpful that output.
> 

Indeed :D

> > 
> > Thanks,
> > Alex
> 
> Best regards,
> Christian Schoenebeck
> 
>