<div dir="ltr">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.<div> </div><div>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.</div><div>Does this make any sense/would be a good idea?</div><div class="gmail_extra"> <div class="gmail_quote">2016-02-19 2:49 GMT+01:00 Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>>: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <div><div class="h5"> > --- > 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 > </div></div>> _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> </div></div>