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

[Jonas Ådahl]

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> .

> 
> 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
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >