[PATCH xserver 1/2] Hold input lock for deviceProc

[Peter Hutterer]

On Thu, Sep 08, 2016 at 11:02:30AM -0600, Keith Packard wrote:
> This ensures that the deviceProc is never called while the input
> thread is processing data from the device.
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  dix/devices.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/dix/devices.c b/dix/devices.c
> index 9de84a6..56aae85 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -399,9 +399,11 @@ EnableDevice(DeviceIntPtr dev, BOOL sendevent)
>          }
>      }
>  
> +    input_lock();
>      if ((*prev != dev) || !dev->inited ||
>          ((ret = (*dev->deviceProc) (dev, DEVICE_ON)) != Success)) {
>          ErrorF("[dix] couldn't enable device %d\n", dev->id);
> +        input_unlock();
>          return FALSE;
>      }
>      dev->enabled = TRUE;
> @@ -410,6 +412,7 @@ EnableDevice(DeviceIntPtr dev, BOOL sendevent)
>      for (prev = &inputInfo.devices; *prev; prev = &(*prev)->next);
>      *prev = dev;
>      dev->next = NULL;
> +    input_unlock();
>  
>      enabled = TRUE;
>      XIChangeDeviceProperty(dev, XIGetKnownProperty(XI_PROP_ENABLED),
> @@ -488,20 +491,20 @@ DisableDevice(DeviceIntPtr dev, BOOL sendevent)
>      if (dev->spriteInfo->paired)
>          dev->spriteInfo->paired = NULL;
>  
> +    input_lock();
>      (void) (*dev->deviceProc) (dev, DEVICE_OFF);
>      dev->enabled = FALSE;
>  
> -    FreeSprite(dev);
> -
>      /* now that the device is disabled, we can reset the event reader's
>       * last.slave */
> -    input_lock();
>      for (other = inputInfo.devices; other; other = other->next) {
>          if (other->last.slave == dev)
>              other->last.slave = NULL;
>      }
>      input_unlock();
>  
> +    FreeSprite(dev);
> +
>      LeaveWindow(dev);
>      SetFocusOut(dev);
>  
> @@ -569,7 +572,9 @@ ActivateDevice(DeviceIntPtr dev, BOOL sendevent)
>      if (!dev || !dev->deviceProc)
>          return BadImplementation;
>  
> +    input_lock();
>      ret = (*dev->deviceProc) (dev, DEVICE_INIT);
> +    input_unlock();

this one isn't needed, as a device that sends events before DEVICE_ON will
simply have all of them discarded anyway. but it won't hurt.

>      dev->inited = (ret == Success);
>      if (!dev->inited)
>          return ret;
> @@ -935,6 +940,8 @@ FreeAllDeviceClasses(ClassesPtr classes)
>   * enable it again and free associated structs. If you want the device to just
>   * be disabled, DisableDevice().
>   * Don't call this function directly, use RemoveDevice() instead.
> + *
> + * Called with input lock held.
>   */
>  static void
>  CloseDevice(DeviceIntPtr dev)
> @@ -1071,6 +1078,11 @@ void
>  AbortDevices(void)
>  {
>      DeviceIntPtr dev;
> +
> +    /* Do not call input_lock as we don't know what
> +     * state the input thread might be in, and that could
> +     * cause a dead-lock.
> +     */
>      nt_list_for_each_entry(dev, inputInfo.devices, next) {
>          if (!IsMaster(dev))
>              (*dev->deviceProc) (dev, DEVICE_ABORT);
> @@ -1135,6 +1147,8 @@ RemoveDevice(DeviceIntPtr dev, BOOL sendevent)
>          flags[dev->id] = XIDeviceDisabled;
>      }
>  
> +    input_lock();
> +
>      prev = NULL;
>      for (tmp = inputInfo.devices; tmp; (prev = tmp), (tmp = next)) {
>          next = tmp->next;
> @@ -1167,6 +1181,8 @@ RemoveDevice(DeviceIntPtr dev, BOOL sendevent)
>          }
>      }
>  
> +    input_unlock();

same with this one, not needed but it won't hurt. 

Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>, thanks.

Cheers,
   Peter

> +
>      if (ret == Success && initialized) {
>          inputInfo.numDevices--;
>          SendDevicePresenceEvent(deviceid, DeviceRemoved);
> -- 
> 2.8.1
>