[PATCH libevdev 3/3] Check max to see if an event type is valid

[Peter Hutterer]

On Mon, Oct 28, 2013 at 02:58:45PM +0100, David Herrmann wrote:
> Hi Peter
> 
> On Thu, Oct 24, 2013 at 7:14 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > There's a gap in the range between EV_SW and EV_LED. Trying to enable one
> > of those bits will segfault.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  libevdev/libevdev.c            | 16 ++++++++++++++--
> >  test/test-libevdev-has-event.c |  6 ++++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index 7bebe32..ca57e26 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -1139,12 +1139,18 @@ libevdev_set_abs_info(struct libevdev *dev, unsigned int code, const struct inpu
> >  LIBEVDEV_EXPORT int
> >  libevdev_enable_event_type(struct libevdev *dev, unsigned int type)
> >  {
> > +       int max;
> > +
> >         if (type > EV_MAX)
> >                 return -1;
> >
> >         if (libevdev_has_event_type(dev, type))
> >                 return 0;
> >
> > +       max = libevdev_event_type_get_max(type);
> > +       if (max == -1)
> > +               return -1;
> > +
> >         set_bit(dev->bits, type);
> >
> >         if (type == EV_REP) {
> > @@ -1158,9 +1164,15 @@ libevdev_enable_event_type(struct libevdev *dev, unsigned int type)
> >  LIBEVDEV_EXPORT int
> >  libevdev_disable_event_type(struct libevdev *dev, unsigned int type)
> >  {
> > +       int max;
> > +
> >         if (type > EV_MAX || type == EV_SYN)
> >                 return -1;
> >
> > +       max = libevdev_event_type_get_max(type);
> > +       if (max == -1)
> > +               return -1;
> > +
> >         clear_bit(dev->bits, type);
> >
> >         return 0;
> > @@ -1192,7 +1204,7 @@ libevdev_enable_event_code(struct libevdev *dev, unsigned int type,
> >
> >         max = type_to_mask(dev, type, &mask);
> >
> > -       if (code > max)
> > +       if (code > max || (int)max == -1)
> 
> type_to_mask() returns an "int" so why is "max" unsigned? The cast
> here looks stupid this way. I'd prefer this:
> 
>        int max;
> 
>        if (max < 0 || code > (unsigned)max)
> 
> To me it's clearer that the <0 + (unsigned) protects against integer
> overflow. But maybe that's just arguing about coding-style. Both works
> so:

"historical" reasons mainly. the first couple of versions of this function
didn't have the max checks so max was only compared to code, hence the
unsigned int. feel free to submit a patch to fix this though, I'm not
attached to either way.

Cheers,
   Peter
 
> Reviewed-by: David Herrmann <dh.herrmann at gmail.com>
> 
> Thanks
> David
> 
> >                 return -1;
> >
> >         set_bit(mask, code);
> > @@ -1219,7 +1231,7 @@ libevdev_disable_event_code(struct libevdev *dev, unsigned int type, unsigned in
> >
> >         max = type_to_mask(dev, type, &mask);
> >
> > -       if (code > max)
> > +       if (code > max || (int)max == -1)
> >                 return -1;
> >
> >         clear_bit(mask, code);
> > diff --git a/test/test-libevdev-has-event.c b/test/test-libevdev-has-event.c
> > index 5af3530..3aca23b 100644
> > --- a/test/test-libevdev-has-event.c
> > +++ b/test/test-libevdev-has-event.c
> > @@ -775,6 +775,9 @@ START_TEST(test_device_enable_bit_invalid)
> >         ck_assert_int_eq(libevdev_enable_event_code(dev, EV_ABS, ABS_MAX + 1, &abs), -1);
> >         ck_assert_int_eq(libevdev_enable_event_code(dev, EV_MAX + 1, ABS_MAX + 1, &abs), -1);
> >         ck_assert_int_eq(libevdev_enable_event_type(dev, EV_MAX + 1), -1);
> > +       /* there's a gap between EV_SW and EV_LED */
> > +       ck_assert_int_eq(libevdev_enable_event_type(dev, EV_LED - 1), -1);
> > +       ck_assert_int_eq(libevdev_enable_event_code(dev, EV_LED - 1, 0, NULL), -1);
> >
> >         ck_assert_int_eq(libevdev_enable_event_code(dev, EV_ABS, ABS_Y, NULL), -1);
> >         ck_assert_int_eq(libevdev_enable_event_code(dev, EV_REP, REP_DELAY, NULL), -1);
> > @@ -843,6 +846,9 @@ START_TEST(test_device_disable_bit_invalid)
> >         rc = test_create_abs_device(&uidev, &dev, 1, &abs, -1);
> >         ck_assert_msg(rc == 0, "Failed to create uinput device: %s", strerror(-rc));
> >
> > +       /* there's a gap between EV_SW and EV_LED */
> > +       ck_assert_int_eq(libevdev_disable_event_type(dev, EV_LED - 1), -1);
> > +       ck_assert_int_eq(libevdev_disable_event_code(dev, EV_LED - 1, 0), -1);
> >         ck_assert_int_eq(libevdev_disable_event_code(dev, EV_ABS, ABS_MAX + 1), -1);
> >         ck_assert_int_eq(libevdev_disable_event_code(dev, EV_MAX + 1, ABS_MAX + 1), -1);
> >         ck_assert_int_eq(libevdev_disable_event_type(dev, EV_MAX + 1), -1);
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/input-tools