[Spice-devel] [PATCH spice-server v3 5/6] Avoid leaking file descriptor for statistics

[Frediano Ziglio]

> 
> On Wed, Nov 16, 2016 at 03:39:15PM +0000, Frediano Ziglio wrote:
> > mmap memory area will remain even if the descriptor is closed.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/stat-file.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/server/stat-file.c b/server/stat-file.c
> > index e9b4208..5f54869 100644
> > --- a/server/stat-file.c
> > +++ b/server/stat-file.c
> > @@ -46,9 +46,11 @@ void stat_file_init(RedStatFile *stat_file, unsigned int
> > max_nodes)
> >          spice_error("statistics shm_open failed, %s", strerror(errno));
> >      }
> >      if (ftruncate(fd, shm_size) == -1) {
> > +        close(fd);
> >          spice_error("statistics ftruncate failed, %s", strerror(errno));

Maybe this is not necessary if we assure spice_error is going
to exit()/abort().

> >      }
> >      stat_file->stat = (SpiceStat *)mmap(NULL, shm_size, PROT_READ |
> >      PROT_WRITE, MAP_SHARED, fd, 0);
> > +    close(fd);
> >      if (stat_file->stat == (SpiceStat *)MAP_FAILED) {
> >          spice_error("statistics mmap failed, %s", strerror(errno));
> >      }
> 
> We never munmap() the mmap data either, dunno if that's intentional?
> 
> Christophe
> 

Yes, many cleanup paths are currently missing in the code.
However we can get rid at least of the descriptor :)
There are currently all the counters still referring to the mmap
area so once we decide to unmap we must be sure there are no pointers
to it.

Frediano