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

[xerpi]

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.

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?

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160219/928bc261/attachment.html>