Hi,
On 10/10/2016 11:10 AM, Emil Velikov wrote:
Hi Hans,
On Wednesday, 5 October 2016, Hans de Goede <[email protected]
<mailto:[email protected]>> wrote:
When the xserver uses threaded input, it keeps a pointer to the InputInfo
passed into xf86AddEnabledDevice and calls pointer->read_input on events.
But when the first enabled device goes away the pInfo we've passed into
xf86AddEnabledDevice gets freed and eventually pInfo->read_input gets
overwritten (or pInfo points to unmapped memory) leading to a segfault.
This commit fixes this by replacing the pInfo passed into
xf86AddEnabledDevice with a pointer to a global InputInfo stored inside
the driver_context struct.
Signed-off-by: Hans de Goede <[email protected]>
---
src/xf86libinput.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/xf86libinput.c b/src/xf86libinput.c
index 21f87f5..485e212 100644
--- a/src/xf86libinput.c
+++ b/src/xf86libinput.c
@@ -86,6 +86,7 @@
struct xf86libinput_driver {
struct libinput *libinput;
+ struct _InputInfoRec InputInfo;
int device_enabled_count;
};
@@ -582,7 +583,17 @@ xf86libinput_on(DeviceIntPtr dev)
if (driver_context.device_enabled_count == 0) {
#if HAVE_THREADED_INPUT
- xf86AddEnabledDevice(pInfo);
+ /*
+ * The xserver keeps a pointer to the InputInfo passed into
+ * xf86AddEnabledDevice and calls pointer->read_input on
+ * events. Thus we cannot simply pass in our current pInfo
+ * as that will be deleted when the current input device
gets
+ * unplugged. Instead pass in a pointer to a global
+ * InputInfo inside the driver_context.
+ */
+ driver_context.InputInfo.fd = pInfo->fd;
Reading the above comment makes me wonder about the lifetime of the fd. I take
it that we're not leaking it atm ?
The fd here actually is libinput's epoll fd, which is shared by all the devices
and gets closed when we destroy the libinput context which we do already when
the
last libinput driven device gets removed.
+ driver_context.InputInfo.read_input = pInfo->read_input;
+ xf86AddEnabledDevice(&driver_context.InputInfo);
#else
/* Can't use xf86AddEnabledDevice on an epollfd */
AddEnabledDevice(pInfo->fd);
Can we use the same storage for the !HAVE_THREADED_INPUT code paths ?
I've not looked closely at the !threaded code paths, but AFAIK the non threaded
paths take just a fd; and then later lookup the pInfo in the list of devices,
so in that case the pInfo must be a real pInfo.
@@ -606,7 +617,7 @@ xf86libinput_off(DeviceIntPtr dev)
if (--driver_context.device_enabled_count == 0) {
#if HAVE_THREADED_INPUT
- xf86RemoveEnabledDevice(pInfo);
+ xf86RemoveEnabledDevice(&driver_context.InputInfo);
#else
RemoveEnabledDevice(pInfo->fd);
#endif
@@ -1923,7 +1934,7 @@ out:
}
static void
-xf86libinput_read_input(InputInfoPtr pInfo)
+xf86libinput_read_input(InputInfoPtr do_not_use)
Worth just dropping the argument and fixing the caller(s)?
This is a callback function called by the server, so we cannot just
change the signature; and other input drivers are likely to actually
use the argument.
Regards,
Hans
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
