[PATCH libinput] Fix leaking device groups
Jonas Ådahl
jadahl at gmail.com
Sun Sep 6 20:48:22 PDT 2015
On Mon, Sep 07, 2015 at 01:11:11PM +1000, Peter Hutterer wrote:
> If a caller has a reference to a device group when the context is destroyed,
> the memory for the group is never released. Calling
> libinput_device_group_unref() will release it and there are no side-effects
> since the group has no back-references. It's inconsistent with the rest of
> libinput though - all other resources get released on libinput_unref().
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Not really sure about this one. If any caller is relying on this behaviour,
> it's a hard break and can cause a crash. otoh this is probably the most
> niche feature we have anyway until the tablet branch is merged. And it does
> bring things in line with the rest of libinput.
I think it makes sense to make things consistent. If we'd deliberately
allow the device group to survive a lost context, then we'd effectively
disallow us to add anything context dependent functionality to the
group, without having to add special lost context semantics.
Considering this, with a couple of minor comments inline, this patch is
Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
>
> src/evdev.c | 18 ++++--------------
> src/libinput-private.h | 11 ++++++++++-
> src/libinput.c | 37 ++++++++++++++++++++++++++++++++++++-
> test/device.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 97 insertions(+), 16 deletions(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 094320e..080e0a1 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -2084,27 +2084,17 @@ static int
> evdev_set_device_group(struct evdev_device *device,
> struct udev_device *udev_device)
> {
> + struct libinput *libinput = device->base.seat->libinput;
> struct libinput_device_group *group = NULL;
> const char *udev_group;
>
> udev_group = udev_device_get_property_value(udev_device,
> "LIBINPUT_DEVICE_GROUP");
> - if (udev_group) {
> - struct libinput_device *d;
> -
> - list_for_each(d, &device->base.seat->devices_list, link) {
> - const char *identifier = d->group->identifier;
> -
> - if (identifier &&
> - strcmp(identifier, udev_group) == 0) {
> - group = d->group;
> - break;
> - }
> - }
> - }
> + if (udev_group)
> + group = libinput_device_group_find_group(libinput, udev_group);
>
> if (!group) {
> - group = libinput_device_group_create(udev_group);
> + group = libinput_device_group_create(libinput, udev_group);
> if (!group)
> return 1;
> libinput_device_set_device_group(&device->base, group);
> diff --git a/src/libinput-private.h b/src/libinput-private.h
> index 8b161cc..be99af5 100644
> --- a/src/libinput-private.h
> +++ b/src/libinput-private.h
> @@ -91,6 +91,8 @@ struct libinput {
> enum libinput_log_priority log_priority;
> void *user_data;
> int refcount;
> +
> + struct list device_group_list;
> };
>
> typedef void (*libinput_seat_destroy_func) (struct libinput_seat *seat);
> @@ -224,6 +226,8 @@ struct libinput_device_group {
> int refcount;
> void *user_data;
> char *identifier; /* unique identifier or NULL for singletons */
> +
> + struct list link;
> };
>
> struct libinput_device {
> @@ -321,7 +325,12 @@ libinput_device_init(struct libinput_device *device,
> struct libinput_seat *seat);
>
> struct libinput_device_group *
> -libinput_device_group_create(const char *identifier);
> +libinput_device_group_create(struct libinput *libinput,
> + const char *identifier);
> +
> +struct libinput_device_group *
> +libinput_device_group_find_group(struct libinput *libinput,
> + const char *identifier);
>
> void
> libinput_device_set_device_group(struct libinput_device *device,
> diff --git a/src/libinput.c b/src/libinput.c
> index e564571..1eb35e2 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -212,6 +212,9 @@ libinput_log_set_handler(struct libinput *libinput,
> }
>
> static void
> +libinput_device_group_destroy(struct libinput_device_group *group);
> +
> +static void
> libinput_post_event(struct libinput *libinput,
> struct libinput_event *event);
>
> @@ -944,6 +947,7 @@ libinput_init(struct libinput *libinput,
> libinput->refcount = 1;
> list_init(&libinput->source_destroy_list);
> list_init(&libinput->seat_list);
> + list_init(&libinput->device_group_list);
>
> if (libinput_timer_subsys_init(libinput) != 0) {
> free(libinput->events);
> @@ -983,6 +987,7 @@ libinput_unref(struct libinput *libinput)
> struct libinput_event *event;
> struct libinput_device *device, *next_device;
> struct libinput_seat *seat, *next_seat;
> + struct libinput_device_group *group, *next_group;
>
> if (libinput == NULL)
> return NULL;
> @@ -1010,6 +1015,13 @@ libinput_unref(struct libinput *libinput)
> libinput_seat_destroy(seat);
> }
>
> + list_for_each_safe(group,
> + next_group,
> + &libinput->device_group_list,
> + link) {
> + libinput_device_group_destroy(group);
> + }
> +
> libinput_timer_subsys_destroy(libinput);
> libinput_drop_destroyed_sources(libinput);
> close(libinput->epoll_fd);
> @@ -1962,7 +1974,8 @@ libinput_device_group_ref(struct libinput_device_group *group)
> }
>
> struct libinput_device_group *
> -libinput_device_group_create(const char *identifier)
> +libinput_device_group_create(struct libinput *libinput,
> + const char *identifier)
> {
> struct libinput_device_group *group;
>
> @@ -1976,12 +1989,33 @@ libinput_device_group_create(const char *identifier)
> if (!group->identifier) {
> free(group);
> group = NULL;
> + goto out;
Why not just return NULL instead of the goto/label?
> }
> }
>
> + list_init(&group->link);
> + list_insert(&libinput->device_group_list, &group->link);
> +
> +out:
> return group;
> }
>
> +struct libinput_device_group *
> +libinput_device_group_find_group(struct libinput *libinput,
> + const char *identifier)
> +{
> + struct libinput_device_group *g;
Initialize to NULL? Especially since you seem to NULL check the return
value after it makes sense to make this more explicit (instead of
relying on the side effect in list_for_each).
Jonas
> +
> + list_for_each(g, &libinput->device_group_list, link) {
> + if (identifier && g->identifier &&
> + streq(g->identifier, identifier)) {
> + return g;
> + }
> + }
> +
> + return g;
> +}
> +
> void
> libinput_device_set_device_group(struct libinput_device *device,
> struct libinput_device_group *group)
> @@ -1993,6 +2027,7 @@ libinput_device_set_device_group(struct libinput_device *device,
> static void
> libinput_device_group_destroy(struct libinput_device_group *group)
> {
> + list_remove(&group->link);
> free(group->identifier);
> free(group);
> }
> diff --git a/test/device.c b/test/device.c
> index 2aa090c..b7fa0e0 100644
> --- a/test/device.c
> +++ b/test/device.c
> @@ -674,6 +674,22 @@ START_TEST(device_context)
> }
> END_TEST
>
> +static int open_restricted(const char *path, int flags, void *data)
> +{
> + int fd;
> + fd = open(path, flags);
> + return fd < 0 ? -errno : fd;
> +}
> +static void close_restricted(int fd, void *data)
> +{
> + close(fd);
> +}
> +
> +const struct libinput_interface simple_interface = {
> + .open_restricted = open_restricted,
> + .close_restricted = close_restricted,
> +};
> +
> START_TEST(device_group_get)
> {
> struct litest_device *dev = litest_current_device();
> @@ -722,6 +738,36 @@ START_TEST(device_group_ref)
> }
> END_TEST
>
> +START_TEST(device_group_leak)
> +{
> + struct libinput *li;
> + struct libinput_device *device;
> + struct libevdev_uinput *uinput;
> + struct libinput_device_group *group;
> +
> + uinput = litest_create_uinput_device("test device", NULL,
> + EV_KEY, BTN_LEFT,
> + EV_KEY, BTN_RIGHT,
> + EV_REL, REL_X,
> + EV_REL, REL_Y,
> + -1);
> +
> + li = libinput_path_create_context(&simple_interface, NULL);
> + device = libinput_path_add_device(li,
> + libevdev_uinput_get_devnode(uinput));
> +
> + group = libinput_device_get_device_group(device);
> + libinput_device_group_ref(group);
> +
> + libinput_path_remove_device(device);
> +
> + libevdev_uinput_destroy(uinput);
> + libinput_unref(li);
> +
> + /* the device group leaks, check valgrind */
> +}
> +END_TEST
> +
> START_TEST(abs_device_no_absx)
> {
> struct libevdev_uinput *uinput;
> @@ -1239,6 +1285,7 @@ litest_setup_tests(void)
>
> litest_add("device:group", device_group_get, LITEST_ANY, LITEST_ANY);
> litest_add_no_device("device:group", device_group_ref);
> + litest_add_no_device("device:group", device_group_leak);
>
> litest_add_no_device("device:invalid devices", abs_device_no_absx);
> litest_add_no_device("device:invalid devices", abs_device_no_absy);
> --
> 2.4.3
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list