[PATCH libevdev] Reset the struct on set_fd

[Peter Hutterer]

On Tue, Oct 22, 2013 at 05:14:59PM +0200, David Herrmann wrote:
> Hi Peter
> 
> On Tue, Oct 22, 2013 at 5:11 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > libevdev_set_fd may fail at a number of points. If it does, it errors out
> > but does not unset previously set fields. Thus, a client may call set_fd
> > again for the same struct but on a different fd and have it succeed.
> > Depending on when set_fd bailed out the first time, some fields may already
> > be set.
> >
> > Thus, reset the whole struct at set_fd time to make sure we're nulled out
> > appropriately.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  libevdev/libevdev.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index 6f203e2..4c1f089 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -110,20 +110,28 @@ log_msg(enum libevdev_log_priority priority,
> >         va_end(args);
> >  }
> >
> > -LIBEVDEV_EXPORT struct libevdev*
> > -libevdev_new(void)
> > +static void
> > +libevdev_reset(struct libevdev *dev)
> >  {
> > -       struct libevdev *dev;
> > -
> > -       dev = calloc(1, sizeof(*dev));
> > -       if (!dev)
> > -               return NULL;
> > +       memset(dev, 0, sizeof(*dev));
> >         dev->fd = -1;
> >         dev->initialized = false;
> >         dev->num_slots = -1;
> >         dev->current_slot = -1;
> >         dev->grabbed = LIBEVDEV_UNGRAB;
> >         dev->sync_state = SYNC_NONE;
> > +}
> > +
> > +LIBEVDEV_EXPORT struct libevdev*
> > +libevdev_new(void)
> > +{
> > +       struct libevdev *dev;
> > +
> > +       dev = malloc(sizeof(*dev));
> > +       if (!dev)
> > +               return NULL;
> > +
> > +       libevdev_reset(dev);
> >
> >         return dev;
> >  }
> > @@ -212,6 +220,8 @@ libevdev_set_fd(struct libevdev* dev, int fd)
> >         } else if (fd < 0)
> >                 return -EBADF;
> >
> > +       libevdev_reset(dev);
> > +
> 
> I think we should call libevdev_reset() in the error-path, too.

added locally:
@@ -360,6 +370,8 @@ libevdev_set_fd(struct libevdev* dev, int fd)

        dev->initialized = true;
 out:
+       if (rc)
+               libevdev_reset(dev);
        return rc ? -errno : 0;
 }

> Otherwise, the state might be initialized with partial values and
> *_get_*() functions will return partial results. But it's a nit-pick
> as we don't guarantee anything about the state after a failed
> set_fd(). 

there's probably some argument that it would allow peeking at info from a
device that fails setup. Not sure how you'd exploit this tbh but resetting
on error is simple enough.

> So I'm fine with either way:
> 
> Reviewed-by: David Herrmann <dh.herrmann at gmail.com>

thanks for the reviews.

Cheers,
   Peter

> >         rc = ioctl(fd, EVIOCGBIT(0, sizeof(dev->bits)), dev->bits);
> >         if (rc < 0)
> >                 goto out;
> > --
> > 1.8.3.1
> >