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

[Bryce Harrington]

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!

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