[PATCH libinput] Do not abort on invalid speed.

[Peter Hutterer]

On Wed, Feb 04, 2015 at 04:45:37PM -0500, Olivier Fourdan wrote:
> Hi Peter,
> 
> Just to clarify, evdev_accel_config_set_speed() calls filter_set_speed()
> which calls accelerator_set_speed() which reaches the assert().
> 
> My patch basically removes the assert() and replaces it by a return false
> so that it fails gracefully.

yeah, I understand that bit but I don't know is how you get here. the
xorg libinput driver calls libinput_device_config_accel_set_speed() which
has the range check. This should be the only entry point for
evdev_accel_config_set_speed() so really, this assert should never trigger.

I tried triggering this through xinput set-prop and it seems to work as
expected with the random values I tried. Can you attach gdb to see the
callstack of how you get there?

Cheers,
   Peter

> ----- Original Message -----
> From: "Olivier Fourdan" <ofourdan at redhat.com>
> To: "Peter Hutterer" <peter.hutterer at who-t.net>
> Cc: wayland-devel at lists.freedesktop.org
> Sent: Wednesday, 4 February, 2015 10:28:22 PM
> Subject: Re: [PATCH libinput] Do not abort on invalid speed.
> 
> Hi Peter,
> 
> Weird, that occurs with xf86-drv-libinput and libinput both from git as of today.
> 
> I don't think this is an uninitialized variable, it's really the Xorg client (read, my code) that sends values out of range and that takes the whole X server down because of the assert()...
> 
> Cheers,
> Olivier
> 
> ----- Original Message -----
> From: "Peter Hutterer" <peter.hutterer at who-t.net>
> To: "Olivier Fourdan" <ofourdan at redhat.com>
> Cc: wayland-devel at lists.freedesktop.org
> Sent: Wednesday, 4 February, 2015 10:15:56 PM
> Subject: Re: [PATCH libinput] Do not abort on invalid speed.
> 
> On Wed, Feb 04, 2015 at 10:34:25AM +0100, Olivier Fourdan wrote:
> > Libinput's accelerator_set_speed() asserts the given speed value is
> > within the [-1.0, 1.0] range.
> > 
> > When using xf86-input-libinput, a client may try to set an invalid speed
> > which will cause the entire Xserver to abort.
> > 
> > Instead of aborting on invalid speed values, simply return a failure
> > (and log a message).
> > 
> > Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> 
> I think this was fixed with b21fbfd947d4bc18b766c46ab1d85a3f8d0977f4 and
> 199cd87b071f6fd0d4a1065ae3d678552259f883, it was caused by an uninitialized
> variable.
> https://bugs.freedesktop.org/show_bug.cgi?id=88899
> 
> libinput_device_config_accel_set_speed() already has the checks for [-1, 1],
> so by the time we get here a speed outside those boundaries is a bug.
> 
> Cheers,
>    Peter
> 
> > ---
> >  src/evdev.c  | 5 ++++-
> >  src/filter.c | 3 ++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/evdev.c b/src/evdev.c
> > index 6e318dc..c96a66b 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -1194,8 +1194,11 @@ evdev_accel_config_set_speed(struct libinput_device *device, double speed)
> >  {
> >  	struct evdev_device *dev = (struct evdev_device *)device;
> >  
> > -	if (!filter_set_speed(dev->pointer.filter, speed))
> > +	if (!filter_set_speed(dev->pointer.filter, speed)) {
> > +		log_bug_libinput(device->seat->libinput,
> > +				 "Invalid speed %f\n", speed);
> >  		return LIBINPUT_CONFIG_STATUS_INVALID;
> > +	}
> >  
> >  	return LIBINPUT_CONFIG_STATUS_SUCCESS;
> >  }
> > diff --git a/src/filter.c b/src/filter.c
> > index 72ef760..91afdcd 100644
> > --- a/src/filter.c
> > +++ b/src/filter.c
> > @@ -260,7 +260,8 @@ accelerator_set_speed(struct motion_filter *filter,
> >  	struct pointer_accelerator *accel_filter =
> >  		(struct pointer_accelerator *)filter;
> >  
> > -	assert(speed >= -1.0 && speed <= 1.0);
> > +	if (speed < -1.0 || speed > 1.0)
> > +		return false;
> >  
> >  	/* delay when accel kicks in */
> >  	accel_filter->threshold = DEFAULT_THRESHOLD - speed/6.0;
> > -- 
> > 2.1.0
> >