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

[Olivier Fourdan]

Hi Peter,

Humm, I suspect that's even more tricky actually, because once a device is disabled, it cannot be enabled again...

With this patch, it won't crash anymore, but it won't re-enable either.

$ xinput list
⎡ Virtual core pointer                    	id=2	[master pointer  (3)]
⎜   ↳ Virtual core XTEST pointer              	id=4	[slave  pointer  (2)]
⎜   ↳ Logitech USB-PS/2 Optical Mouse         	id=11	[slave  pointer  (2)]
⎜   ↳ TPPS/2 IBM TrackPoint                   	id=15	[slave  pointer  (2)]
⎜   ↳ SynPS/2 Synaptics TouchPad              	id=14	[slave  pointer  (2)]
⎜   ↳ HID 04d9:1400                           	id=10	[slave  pointer  (2)]
⎣ Virtual core keyboard                   	id=3	[master keyboard (2)]
    ↳ Virtual core XTEST keyboard             	id=5	[slave  keyboard (3)]
    ↳ Power Button                            	id=6	[slave  keyboard (3)]
    ↳ Video Bus                               	id=7	[slave  keyboard (3)]
    ↳ Sleep Button                            	id=8	[slave  keyboard (3)]
    ↳ HID 04d9:1400                           	id=9	[slave  keyboard (3)]
    ↳ Integrated Camera                       	id=12	[slave  keyboard (3)]
    ↳ AT Translated Set 2 keyboard            	id=13	[slave  keyboard (3)]
    ↳ ThinkPad Extra Buttons                  	id=16	[slave  keyboard (3)]

$ xinput enable 14
X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  131 (XInputExtension)
  Minor opcode of failed request:  57 ()
  Serial number of failed request:  20
  Current serial number in output stream:  21

So it seems with the xf86-input-libinput driver, once I disabled a device I cannot re-enable it anymore.

Cheers,
Olivier

----- Original Message -----
From: "Olivier Fourdan" <ofourdan at redhat.com>
To: "Peter Hutterer" <peter.hutterer at who-t.net>
Cc: xorg-devel at lists.x.org
Sent: Wednesday, February 25, 2015 9:07:24 AM
Subject: Re: [RFC PATCH xf86-input-libinput v2] Do not crash if the device is invalid

Hi Peter,

On 25/02/15 03:41, Peter Hutterer wrote:
> 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.

Yes, that's correct, the crash occurs on a disabled device indeed.

> 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:

That new patch works as expected.

>
>  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,

Is "info" sufficient, shouldn't this be a warning instead?

> +			    "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)
>

Looks good to me anyway :)

Cheers,
Olivier
_______________________________________________
xorg-devel at lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel