[PATCH libevdev 2/5] Dynamically allocate the slot values
Benjamin Tissoires
benjamin.tissoires at gmail.com
Mon Mar 3 20:41:37 PST 2014
On Mon, Mar 3, 2014 at 10:19 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> On Mon, Mar 03, 2014 at 10:37:50AM -0500, Benjamin Tissoires wrote:
>> On Thu, Feb 27, 2014 at 1:27 AM, Peter Hutterer
>> <peter.hutterer at who-t.net> wrote:
>> > Instead of relying on a static MAX_SLOTS array, allocated it based on the
>> > number of slots we have on the device. The previous checks for MAX_SLOTS were
>> > incomplete, causing out-of-bound reads.
>> >
>> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>> > ---
>> > libevdev/libevdev-int.h | 2 +-
>> > libevdev/libevdev.c | 41 +++++++++++++++++++++++++++++++----------
>> > 2 files changed, 32 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
>> > index 847fe56..42141ca 100644
>> > --- a/libevdev/libevdev-int.h
>> > +++ b/libevdev/libevdev-int.h
>> > @@ -95,7 +95,7 @@ struct libevdev {
>> > unsigned long led_values[NLONGS(LED_CNT)];
>> > unsigned long sw_values[NLONGS(SW_CNT)];
>> > struct input_absinfo abs_info[ABS_CNT];
>> > - int mt_slot_vals[MAX_SLOTS][ABS_MT_CNT];
>> > + int *mt_slot_vals; /* [num_slots * ABS_MT_CNT] */
>> > int num_slots; /**< valid slots in mt_slot_vals */
>> > int current_slot;
>> > int rep_values[REP_CNT];
>> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
>> > index 36359d4..4f5062d 100644
>> > --- a/libevdev/libevdev.c
>> > +++ b/libevdev/libevdev.c
>> > @@ -38,6 +38,21 @@
>> >
>> > static int sync_mt_state(struct libevdev *dev, int create_events);
>> >
>> > +static inline int*
>> > +slot_value(const struct libevdev *dev, int slot, int axis)
>> > +{
>> > + if (slot > dev->num_slots) {
>> > + log_bug("Slot %d exceeds number of slots (%d)\n", slot, dev->num_slots);
>> > + slot = 0;
>> > + }
>> > + if (axis < ABS_MT_MIN || axis > ABS_MT_MAX) {
>> > + log_bug("MT axis %d is outside the valid range [%d,%d]\n",
>> > + axis, ABS_MT_MIN, ABS_MT_MAX);
>> > + axis = ABS_MT_MIN;
>> > + }
>> > + return &dev->mt_slot_vals[slot * ABS_MT_CNT + axis - ABS_MT_MIN];
>> > +}
>> > +
>>
>> I am a little bit puzzled by this one.
>> If the slot is not valid or if the axis is not valid, we return a
>> random address, which may be used to overwrite actual data.
>> Also, in some places later, the code actually checks if slot is valid,
>> which means that we do not have the log_bug() message there.
>>
>> How about splitting this function in 2 (or 3), 1 (or 2) to check the
>> validity of slots and axis, and the last would return the pointer,
>> (maybe return NULL if there is an error in slots or axis)?
>
> the reason for this approach is that this is a function used exclusively
> internally and I don't want to have error checks around every instance, only
> where we need to worry about user input. elsewhere, we can assume that the
> input data is correct (after the basic checks we need anyway).
Ok, but with this implementation, you do run the checks every call,
while 98% of them are not required.
>
> so in theory, this should never happen. when it does, return something that
> we can work with nonetheless (as opposed to potentially bringing your
> compositor down), log a bug and continue, albeit slightly wobbly now. this
> is a case where having a unlikely() compiler macro would be useful since we
> don't expect this to ever be in the hot path.
Agree for unlikely(). I may say something dumb, but can't we just skip
the tests in slot_value(), put big warnings around it (comments), and
put extra care where there is no guarantee that the slots or axis are
not correct.
I know tests do not cost that much, but still, in sync_mt_state in the
worse case, you are making (3 * 3 * ABS_MT_CNT * min(dev->num_slots,
MAX_SLOTS)) = 8100 (ok, there is no mt device with 60 touches which
exports all the axes and the axes need to be changed all at once). Or
maybe we count on the compiler to prefetch the branches?
>
> fwiw, when it comes to returning something random, ABS_MT_POSITION_X in slot
> 0 is the safest to return, it'll exist on almost every touch device.
so why do you return ABS_MT_TOUCH_MAJOR? :)
The code set ABS_MT_MIN as the axis, so this gives us the first
MT_AXIS in the slot offset, and the first one is ABS_MT_TOUCH_MAJOR,
not ABS_MT_POSITION_X...
Cheers,
Benjamin
>
> Cheers,
> Peter
>
>
>> > init_event_queue(struct libevdev *dev)
>> > {
>> > @@ -136,6 +151,7 @@ libevdev_reset(struct libevdev *dev)
>> > free(dev->name);
>> > free(dev->phys);
>> > free(dev->uniq);
>> > + free(dev->mt_slot_vals);
>> > memset(dev, 0, sizeof(*dev));
>> > dev->fd = -1;
>> > dev->initialized = false;
>> > @@ -187,6 +203,7 @@ libevdev_free(struct libevdev *dev)
>> > free(dev->name);
>> > free(dev->phys);
>> > free(dev->uniq);
>> > + free(dev->mt_slot_vals);
>> > queue_free(dev);
>> > free(dev);
>> > }
>> > @@ -369,6 +386,11 @@ libevdev_set_fd(struct libevdev* dev, int fd)
>> > if (i == ABS_MT_SLOT &&
>> > !libevdev_has_event_code(dev, EV_ABS, ABS_MT_SLOT - 1)) {
>> > dev->num_slots = abs_info.maximum + 1;
>> > + dev->mt_slot_vals = calloc(dev->num_slots * ABS_MT_CNT, sizeof(int));
>> > + if (!dev->mt_slot_vals) {
>> > + rc = -ENOMEM;
>> > + goto out;
>> > + }
>> > dev->current_slot = abs_info.value;
>> > }
>> >
>> > @@ -579,14 +601,14 @@ sync_mt_state(struct libevdev *dev, int create_events)
>> > if (!libevdev_has_event_code(dev, EV_ABS, j))
>> > continue;
>> >
>> > - if (dev->mt_slot_vals[i][jdx] == mt_state[jdx].val[i])
>> > + if (*slot_value(dev, i, j) == mt_state[jdx].val[i])
>> > continue;
>> >
>> > if (create_events) {
>> > ev = queue_push(dev);
>> > init_event(dev, ev, EV_ABS, j, mt_state[jdx].val[i]);
>> > }
>> > - dev->mt_slot_vals[i][jdx] = mt_state[jdx].val[i];
>> > + *slot_value(dev, i, j) = mt_state[jdx].val[i];
>> > }
>> > }
>> >
>> > @@ -667,14 +689,14 @@ update_mt_state(struct libevdev *dev, const struct input_event *e)
>> > /* sync abs_info with the current slot values */
>> > for (i = ABS_MT_SLOT + 1; i <= ABS_MT_MAX; i++) {
>> > if (libevdev_has_event_code(dev, EV_ABS, i))
>> > - dev->abs_info[i].value = dev->mt_slot_vals[dev->current_slot][i - ABS_MT_MIN];
>> > + dev->abs_info[i].value = *slot_value(dev, dev->current_slot, i);
>> > }
>> >
>> > return 0;
>> > } else if (dev->current_slot == -1)
>> > return 1;
>> >
>> > - dev->mt_slot_vals[dev->current_slot][e->code - ABS_MT_MIN] = e->value;
>> > + *slot_value(dev, dev->current_slot, e->code) = e->value;
>> >
>> > return 0;
>> > }
>> > @@ -1070,13 +1092,13 @@ libevdev_get_slot_value(const struct libevdev *dev, unsigned int slot, unsigned
>> > if (!libevdev_has_event_type(dev, EV_ABS) || !libevdev_has_event_code(dev, EV_ABS, code))
>> > return 0;
>> >
>> > - if (dev->num_slots < 0 || slot >= (unsigned int)dev->num_slots || slot >= MAX_SLOTS)
>> > + if (dev->num_slots < 0 || slot >= (unsigned int)dev->num_slots)
>> > return 0;
>> >
>> > if (code > ABS_MT_MAX || code < ABS_MT_MIN)
>> > return 0;
>> >
>> > - return dev->mt_slot_vals[slot][code - ABS_MT_MIN];
>> > + return *slot_value(dev, slot, code);
>> > }
>> >
>> > LIBEVDEV_EXPORT int
>> > @@ -1085,7 +1107,7 @@ libevdev_set_slot_value(struct libevdev *dev, unsigned int slot, unsigned int co
>> > if (!libevdev_has_event_type(dev, EV_ABS) || !libevdev_has_event_code(dev, EV_ABS, code))
>> > return -1;
>> >
>> > - if (dev->num_slots == -1 || slot >= (unsigned int)dev->num_slots || slot >= MAX_SLOTS)
>> > + if (dev->num_slots == -1 || slot >= (unsigned int)dev->num_slots)
>> > return -1;
>> >
>> > if (code > ABS_MT_MAX || code < ABS_MT_MIN)
>> > @@ -1097,8 +1119,7 @@ libevdev_set_slot_value(struct libevdev *dev, unsigned int slot, unsigned int co
>> > dev->current_slot = value;
>> > }
>> >
>> > - dev->mt_slot_vals[slot][code - ABS_MT_MIN] = value;
>> > -
>> > + *slot_value(dev, slot, code) = value;
>> >
>> > return 0;
>> > }
>> > @@ -1109,7 +1130,7 @@ libevdev_fetch_slot_value(const struct libevdev *dev, unsigned int slot, unsigne
>> > if (libevdev_has_event_type(dev, EV_ABS) &&
>> > libevdev_has_event_code(dev, EV_ABS, code) &&
>> > dev->num_slots >= 0 &&
>> > - slot < (unsigned int)dev->num_slots && slot < MAX_SLOTS) {
>> > + slot < (unsigned int)dev->num_slots) {
>> > *value = libevdev_get_slot_value(dev, slot, code);
>> > return 1;
>> > } else
>> > --
>> > 1.8.4.2
>> >
>> > _______________________________________________
>> > Input-tools mailing list
>> > Input-tools at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list