[PATCH xf86-input-libinput] Fix crash when using threaded input and the first device goes away

[Hans de Goede]

Hi,

On 10/10/2016 11:10 AM, Emil Velikov wrote:
> Hi Hans,
>
> On Wednesday, 5 October 2016, Hans de Goede <hdegoede at redhat.com <mailto:hdegoede at redhat.com>> 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 <hdegoede at redhat.com>
>     ---
>      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