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

[Peter Hutterer]

On Thu, Jun 07, 2012 at 07:47:02AM -0700, Chase Douglas wrote:
> 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 :).

I don't think so. Chris Halse-Rogers had a bug a while ago that I thought
was this issue but it turned out to be something else. nonetheless, the bug
is there, even if it's likely never triggered.

> >  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.

fixed, thanks.
 
> > +}
> > +
> > +
> >  /**
> >   * 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.

well, "really work" is a stretch. It's introducing consistent
behaviour into a broken design. Under normal use, this bug _will_ be
triggered if the server is running long enough.

some numbers: synaptics has an axis range of 3040 vertically, so that makes 
just over 700 000 vertical scroll movements covering the height of the pad
before we overflow.  Not sure what the average scroll events/day ratio is
though, I suspect we're still talking about a year or more of only scrolling
in one direction ever. If we're looking at higher resolution devices, this
number goes down significantly though. An Intuos4 has a vertical range of
~28000, Cintiqs are way north of that.
Unlikely, not impossible though.

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

thanks!

Cheers,
  Peter