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

[Olivier Fourdan]

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