[PATCH libinput] Fix leaking device groups
Peter Hutterer
peter.hutterer at who-t.net
Sun Sep 6 22:26:53 PDT 2015
On Mon, Sep 07, 2015 at 11:48:22AM +0800, Jonas Ådahl wrote:
> 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).
both fixed, thanks.
Cheers,
Peter
> > +
> > + 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