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 <jad...@gmail.com> .
Looks fine to me as well. Reviewed-by: Bryce Harrington <br...@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 <jad...@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@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > > > > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel