[RFC PATCH xf86-input-libinput v2] Do not crash if the device is invalid

[Peter Hutterer]

On Tue, Feb 24, 2015 at 04:12:16PM +0100, Olivier Fourdan wrote:
> https://bugs.freedesktop.org/show_bug.cgi?id=89296
> 
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> ---
> 
>  v2: Move the test to LibinputSetProperty() and do it only once
> 
>  src/libinput.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libinput.c b/src/libinput.c
> index 9613fbd..ac14adb 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -1550,8 +1550,14 @@ static int
>  LibinputSetProperty(DeviceIntPtr dev, Atom atom, XIPropertyValuePtr val,
>                   BOOL checkonly)
>  {
> +	InputInfoPtr pInfo = dev->public.devicePrivate;
> +	struct xf86libinput *driver_data = pInfo->private;
> +	struct libinput_device *device = driver_data->device;
>  	int rc;
>  
> +	if (device == NULL)
> +		return XI_BadDevice;
> +
>  	if (atom == prop_tap)
>  		rc = LibinputSetPropertyTap(dev, atom, val, checkonly);
>  	else if (atom == prop_calibration)
> -- 
> 2.1.0

this turned out to be a bigger problem than expected, it happens whenever a
property is set on a disabled device. It's quite easy to reproduce:

   xinput disable <device name>
   xinput set-prop <device name> "libinput Left Handed Enabled" 1

I suspect what's happening in XFCE is that you immediately set all
properties on all devices without waiting for them to be enabled first.
Historically, that didn't matter because drivers never had to worry about
this. In the libinput driver this matters because we need libinput to
refresh the FDs to check what properties we have.

So the patch above is needed but we now run into the issue that this case
is not defined in the protocol. I think BadDevice sends the wrong message,
the device is valid after all. BadValue likewise, so I'm going with
BadMatch.

Either way, this can't be fixed in the driver, so we need the clients to
work around this and only set properties once the device was enabled.
Updated patch:

>From 97857beac4621f79c6b853c37357573ae11038f7 Mon Sep 17 00:00:00 2001
From: Olivier Fourdan <ofourdan at redhat.com>
Date: Tue, 24 Feb 2015 16:12:16 +0100
Subject: [PATCH xf86-input-libinput] Ignore property changes if the device is
 disabled

If the device is present but disabled, the server will still call into
SetProperty. We don't have a libinput device to back it up in this case,
causing a null-pointer dereference.

This is a bug specific to this driver that cannot easily be fixed. All other
drivers can handle property changes even if no device is present, here we rely
on libinput to make the final call. But without a device path/fd we don't have
a libinput reference

The protocol doesn't mention this case, so let's pick BadMatch as the least
wrong error code. And put a warning in the log, this needs a workaround in the
client.

Also, if we get here and the device is on, then that's definitely a bug, warn
about that.

https://bugs.freedesktop.org/show_bug.cgi?id=89296

Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
---
 src/libinput.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/libinput.c b/src/libinput.c
index eee3bfb..482e0d7 100644
--- a/src/libinput.c
+++ b/src/libinput.c
@@ -1550,8 +1550,20 @@ static int
 LibinputSetProperty(DeviceIntPtr dev, Atom atom, XIPropertyValuePtr val,
                  BOOL checkonly)
 {
+	InputInfoPtr pInfo = dev->public.devicePrivate;
+	struct xf86libinput *driver_data = pInfo->private;
+	struct libinput_device *device = driver_data->device;
 	int rc;
 
+	if (device == NULL && checkonly) {
+		BUG_WARN(dev->public.on);
+		xf86IDrvMsg(pInfo, X_INFO,
+			    "SetProperty on %d called but device is disabled.\n"
+			    "This driver cannot change properties on a disabled device\n",
+			    atom);
+		return BadMatch;
+	}
+
 	if (atom == prop_tap)
 		rc = LibinputSetPropertyTap(dev, atom, val, checkonly);
 	else if (atom == prop_calibration)
-- 
2.1.0