[PATCH] dix: if the scroll valuator reaches INT_MAX, reset to 0

[Chase Douglas]

On 06/07/2012 12:00 AM, Peter Hutterer wrote:
> Too much scrolling down may eventually trigger an overflow of the valuator.
> If this happens, reset the valuator to 0 and skip this event for button
> emulation. Clients will have to figure out a way to deal with this, but a
> scroll event from (close to) INT_MAX to 0 is a hint of that it needs to be
> ignored.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
> Not 100% sure if we really need this but it's one of the design issues with
> XI 2.1. Scrolling down is more common, so eventually we may overflow the
> axis. This patch simply resets it to 0 and pretends nothing happened.
> 
> We don't have overflow protection on any other relative axis, but they're
> less likely to trigger this. x/y are screen coordinate clipped, we don't
> really have devices with rx/ry, and the wheels and dials are smooth
> scrolling now. So smooth scrolling axes are the only ones in danger of
> triggering this, even though it takes quite some effort to even get close to
> triggering this.

Has someone triggered this? Or did you just think about it :).

> Cheers,
>   Peter
> 
>  dix/getevents.c |   41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 32ef225..d343ecf 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -35,6 +35,7 @@
>  #include <X11/keysym.h>
>  #include <X11/Xproto.h>
>  #include <math.h>
> +#include <limits.h>
>  
>  #include "misc.h"
>  #include "resource.h"
> @@ -756,6 +757,30 @@ clipAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
>      }
>  }
>  
> +static void
> +add_to_scroll_valuator(DeviceIntPtr dev, ValuatorMask *mask, int valuator, double value)
> +{
> +    double v;
> +
> +    if (!valuator_mask_fetch_double(mask, valuator, &v))
> +        return;
> +
> +    /* protect against scrolling overflow. INT_MAX for double, because
> +     * we'll eventually write this as 32.32 fixed point */
> +    if ((value > 0 && v > INT_MAX - value) || (value < 0 && v < INT_MIN - value)) {
> +        v = 0;
> +
> +        /* reset last.scroll to avoid a button storm */
> +        valuator_mask_set_double(dev->last.scroll, valuator, 0);
> +    }
> +    else
> +        v += value;
> +    valuator_mask_set_double(mask, valuator, v);
> +
> +

Extra whitespace ^^. I would also add a line between the if..else..
block and the mask setting. It threw me off trying to read what belonged
in the blocks.

> +}
> +
> +
>  /**
>   * Move the device's pointer by the values given in @valuators.
>   *
> @@ -774,13 +799,17 @@ moveRelative(DeviceIntPtr dev, ValuatorMask *mask)
>  
>          if (!valuator_mask_isset(mask, i))
>              continue;
> -        val += valuator_mask_get_double(mask, i);
> +
> +        add_to_scroll_valuator(dev, mask, i, val);
> +
>          /* x & y need to go over the limits to cross screens if the SD
>           * isn't currently attached; otherwise, clip to screen bounds. */
>          if (valuator_get_mode(dev, i) == Absolute &&
> -            ((i != 0 && i != 1) || clip_xy))
> +            ((i != 0 && i != 1) || clip_xy)) {
> +            val = valuator_mask_get_double(mask, i);
>              clipAxis(dev, i, &val);
> -        valuator_mask_set_double(mask, i, val);
> +            valuator_mask_set_double(mask, i, val);
> +        }
>      }
>  }
>  
> @@ -1560,7 +1590,7 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
>       * necessary. This only needs to cater for the XIScrollFlagPreferred
>       * axis (if more than one scrolling axis is present) */
>      if (type == ButtonPress) {
> -        double val, adj;
> +        double adj;
>          int axis;
>          int h_scroll_axis = -1;
>          int v_scroll_axis = -1;
> @@ -1596,8 +1626,7 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type,
>  
>          if (adj != 0.0 && axis != -1) {
>              adj *= pDev->valuator->axes[axis].scroll.increment;
> -            val = valuator_mask_get_double(&mask, axis) + adj;
> -            valuator_mask_set_double(&mask, axis, val);
> +            add_to_scroll_valuator(pDev, &mask, axis, adj);
>              type = MotionNotify;
>              buttons = 0;
>              flags |= POINTER_EMULATED;

This looks reasonable to me, but I don't feel knowledgeable enough about
XI 2.1 to know that this solution will really work. I trust you know
what you are doing there.

Reviewed-by: Chase Douglas <chase.douglas at canonical.com>