Re: [PATCH xf86-input-libinput] Swap the registered input device on DEVICE_OFF when needed

2016-10-18 Thread Hans de Goede 

Hi,

On 18-10-16 03:47, Peter Hutterer wrote:

If we don't swap out the pInfo previously passed to xf86AddEnabledDevice(),
the thread eventually calls read_input on a struct that has been deleted.
Avoid this by swapping out the to-be-destroyed pInfo with the first one we
find.

Reproducer: sudo udevadm trigger --type=devices --action=add

Signed-off-by: Peter Hutterer 


Patch looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans



---
Changes to the RFC:
- checking for the driver now

 src/xf86libinput.c | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 69f7ae3..061e495 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -87,6 +87,7 @@
 struct xf86libinput_driver {
struct libinput *libinput;
int device_enabled_count;
+   void *registered_InputInfoPtr;
 };

 static struct xf86libinput_driver driver_context;
@@ -583,6 +584,7 @@ xf86libinput_on(DeviceIntPtr dev)
if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
xf86AddEnabledDevice(pInfo);
+   driver_context.registered_InputInfoPtr = pInfo;
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);
@@ -1131,6 +1133,39 @@ xf86libinput_init(DeviceIntPtr dev)
return 0;
 }

+static bool
+is_libinput_device(InputInfoPtr pInfo)
+{
+   char *driver;
+   BOOL rc;
+
+   driver = xf86CheckStrOption(pInfo->options, "driver", "");
+   rc = strcmp(driver, "libinput") == 0;
+   free(driver);
+
+   return rc;
+}
+
+static void
+swap_registered_device(InputInfoPtr pInfo)
+{
+   InputInfoPtr next;
+
+   if (pInfo != driver_context.registered_InputInfoPtr)
+   return;
+
+   next = xf86FirstLocalDevice();
+   while (next == pInfo || !is_libinput_device(next))
+   next = next->next;
+
+   input_lock();
+   xf86RemoveEnabledDevice(pInfo);
+   if (next) /* shouldn't ever be NULL anyway */
+   xf86AddEnabledDevice(next);
+   driver_context.registered_InputInfoPtr = next;
+   input_unlock();
+}
+
 static void
 xf86libinput_destroy(DeviceIntPtr dev)
 {
@@ -1138,6 +1173,17 @@ xf86libinput_destroy(DeviceIntPtr dev)
struct xf86libinput *driver_data = pInfo->private;
struct xf86libinput_device *shared_device = driver_data->shared_device;

+   /* If the device being destroyed is the one we used for
+* xf86AddEnabledDevice(), we need to swap it out for one that is
+* still live. xf86AddEnabledDevice() buffers some data and once the
+* deletes pInfo (when DEVICE_OFF completes) the thread will keep
+* calling that struct's read_input because we never removed it.
+* Avoid this by removing ours and substituting one that's still
+* valid, the fd is the same anyway (libinput's epollfd).
+*/
+   if (driver_context.device_enabled_count > 0)
+   swap_registered_device(pInfo);
+
xorg_list_del(_data->shared_device_link);

if (driver_data->tablet_tool)


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xf86-input-libinput] Swap the registered input device on DEVICE_OFF when needed

2016-10-17 Thread Peter Hutterer 
If we don't swap out the pInfo previously passed to xf86AddEnabledDevice(),
the thread eventually calls read_input on a struct that has been deleted.
Avoid this by swapping out the to-be-destroyed pInfo with the first one we
find.

Reproducer: sudo udevadm trigger --type=devices --action=add

Signed-off-by: Peter Hutterer 
---
Changes to the RFC:
- checking for the driver now

 src/xf86libinput.c | 46 ++
 1 file changed, 46 insertions(+)

diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 69f7ae3..061e495 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -87,6 +87,7 @@
 struct xf86libinput_driver {
struct libinput *libinput;
int device_enabled_count;
+   void *registered_InputInfoPtr;
 };
 
 static struct xf86libinput_driver driver_context;
@@ -583,6 +584,7 @@ xf86libinput_on(DeviceIntPtr dev)
if (driver_context.device_enabled_count == 0) {
 #if HAVE_THREADED_INPUT
xf86AddEnabledDevice(pInfo);
+   driver_context.registered_InputInfoPtr = pInfo;
 #else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);
@@ -1131,6 +1133,39 @@ xf86libinput_init(DeviceIntPtr dev)
return 0;
 }
 
+static bool
+is_libinput_device(InputInfoPtr pInfo)
+{
+   char *driver;
+   BOOL rc;
+
+   driver = xf86CheckStrOption(pInfo->options, "driver", "");
+   rc = strcmp(driver, "libinput") == 0;
+   free(driver);
+
+   return rc;
+}
+
+static void
+swap_registered_device(InputInfoPtr pInfo)
+{
+   InputInfoPtr next;
+
+   if (pInfo != driver_context.registered_InputInfoPtr)
+   return;
+
+   next = xf86FirstLocalDevice();
+   while (next == pInfo || !is_libinput_device(next))
+   next = next->next;
+
+   input_lock();
+   xf86RemoveEnabledDevice(pInfo);
+   if (next) /* shouldn't ever be NULL anyway */
+   xf86AddEnabledDevice(next);
+   driver_context.registered_InputInfoPtr = next;
+   input_unlock();
+}
+
 static void
 xf86libinput_destroy(DeviceIntPtr dev)
 {
@@ -1138,6 +1173,17 @@ xf86libinput_destroy(DeviceIntPtr dev)
struct xf86libinput *driver_data = pInfo->private;
struct xf86libinput_device *shared_device = driver_data->shared_device;
 
+   /* If the device being destroyed is the one we used for
+* xf86AddEnabledDevice(), we need to swap it out for one that is
+* still live. xf86AddEnabledDevice() buffers some data and once the
+* deletes pInfo (when DEVICE_OFF completes) the thread will keep
+* calling that struct's read_input because we never removed it.
+* Avoid this by removing ours and substituting one that's still
+* valid, the fd is the same anyway (libinput's epollfd).
+*/
+   if (driver_context.device_enabled_count > 0)
+   swap_registered_device(pInfo);
+
xorg_list_del(_data->shared_device_link);
 
if (driver_data->tablet_tool)
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel