[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [RFC PATCH xf86-input-libinput v2] Do not crash if the device is invalid
From: Olivier Fourdan <ofourdan () redhat ! com>
Date: 2015-02-25 9:12:22
Message-ID: 1314654951.4552063.1424855542947.JavaMail.zimbra () redhat ! com
[Download RAW message or body]
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@redhat.com>
To: "Peter Hutterer" <peter.hutterer@who-t.net>
Cc: xorg-devel@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@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@redhat.com>
> Signed-off-by: Peter Hutterer <peter.hutterer@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@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic