[PATCH] server: Fix shm_create_pool size fail path fd leak

[Pekka Paalanen]

On Mon, 22 Feb 2016 14:41:44 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Mon, Feb 22, 2016 at 01:23:00PM +0800, Jonas Ådahl wrote:
> > On Fri, Feb 19, 2016 at 11:13:25AM +0100, xerpi wrote:  
> > > I was just reading the source when I found it (no valgrind involved). So
> > > as wl_connection_destroy() already takes care of that, my patch is
> > > pointless.  
> > 
> > I was wrong. Your patch is correct, and we do actually leak it.  The fd
> > list that is closed automatically is just the fds that are collected
> > during reading but yet put in a wl_closure. I went a head and wrote a
> > test case for this, just to be sure; I'll send it as a reply to this
> > E-mail.
> > 
> > So consider the patch Reviewed-by: Jonas Ådahl <jadahl at gmail.com> .  
> 
> Looks fine to me as well.
> 
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> 
> I think this is fine to land as-is, but it'd be nice to land it along
> with the test case so will wait a bit for the compilation issue there to
> get squared away.  Thanks for doing the test case btw!

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq

> > > My final idea would be to add support to fds that have the F_SEAL_SHRINK
> > > seal set (see memfd_create(2) and fcntl(2)), so if a client creates
> > > shm_pool with the shrink seal set, and the fd size (fstat(2)) is already >=
> > > size arg passed to wl_shm_create_pool, we no longer would need to set
> > > SIGBUS handlers when the compositor needs to access that shm memory.
> > > Does this make any sense/would be a good idea?  
> > 
> > Currently the server will close the fd immediately after mapping the
> > memory associated with it. How will F_SEAL_SHRINK protect from the
> > client unmapping the memory and closing the fd on its side, without the
> > server having its own fd un-closed?
> > 
> > 
> > Jonas
> >   
> > > 
> > > 2016-02-19 2:49 GMT+01:00 Jonas Ådahl <jadahl at gmail.com>:
> > >   
> > > > On Thu, Feb 18, 2016 at 11:59:29PM +0100, Sergi Granell wrote:  
> > > > > If the client passed a size <= 0 to shm_create_pool, it would
> > > > > go to err_free, which wouldn't close the fd, and thus leave it opened.
> > > > >
> > > > > We can also move the size check before the struct wl_shm_pool
> > > > > malloc, so in case the client passes a wrong size, it won't
> > > > > do an unnecessary malloc and then free.  
> > > >
> > > > Did you detect this using valgrind or something like that? Because IIRC,
> > > > we collect all transmitted file descriptors and close them when the
> > > > client is destroyed (see wl_connection_destory()). Since we post an
> > > > error, the client will eventually be destroyed, so we shouldn't leak
> > > > the fd here already.
> > > >
> > > >
> > > > Jonas
> > > >  
> > > > > ---
> > > > >  src/wayland-shm.c | 20 ++++++++++----------
> > > > >  1 file changed, 10 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> > > > > index a4343a4..81bf657 100644
> > > > > --- a/src/wayland-shm.c
> > > > > +++ b/src/wayland-shm.c
> > > > > @@ -230,17 +230,17 @@ shm_create_pool(struct wl_client *client, struct  
> > > > wl_resource *resource,  
> > > > >  {
> > > > >       struct wl_shm_pool *pool;
> > > > >
> > > > > -     pool = malloc(sizeof *pool);
> > > > > -     if (pool == NULL) {
> > > > > -             wl_client_post_no_memory(client);
> > > > > -             goto err_close;
> > > > > -     }
> > > > > -
> > > > >       if (size <= 0) {
> > > > >               wl_resource_post_error(resource,
> > > > >                                      WL_SHM_ERROR_INVALID_STRIDE,
> > > > >                                      "invalid size (%d)", size);
> > > > > -             goto err_free;
> > > > > +             goto err_close;
> > > > > +     }
> > > > > +
> > > > > +     pool = malloc(sizeof *pool);
> > > > > +     if (pool == NULL) {
> > > > > +             wl_client_post_no_memory(client);
> > > > > +             goto err_close;
> > > > >       }
> > > > >
> > > > >       pool->refcount = 1;
> > > > > @@ -251,7 +251,7 @@ shm_create_pool(struct wl_client *client, struct  
> > > > wl_resource *resource,  
> > > > >               wl_resource_post_error(resource,
> > > > >                                      WL_SHM_ERROR_INVALID_FD,
> > > > >                                      "failed mmap fd %d", fd);
> > > > > -             goto err_close;
> > > > > +             goto err_free;
> > > > >       }
> > > > >       close(fd);
> > > > >
> > > > > @@ -270,10 +270,10 @@ shm_create_pool(struct wl_client *client, struct  
> > > > wl_resource *resource,  
> > > > >
> > > > >       return;
> > > > >
> > > > > -err_close:
> > > > > -     close(fd);
> > > > >  err_free:
> > > > >       free(pool);
> > > > > +err_close:
> > > > > +     close(fd);
> > > > >  }
> > > > >
> > > > >  static const struct wl_shm_interface shm_interface = {
> > > > > --
> > > > > 2.7.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160226/9dbfff1c/attachment.sig>