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

[Bryce Harrington]

On Mon, Feb 22, 2016 at 02:41:44PM -0800, Bryce Harrington 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!

I'd still love to see the test case but this is fine to land now.
Hopefully the updated test case can land in time.

Pushed:
To ssh://git.freedesktop.org/git/wayland/wayland
   ba2ee84..5fe7e7c  master -> master

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