Seeing crashes likely from negative touch indexes, check for them

Seeing crashes likely from negative touch indexes, check for them

BUG=452602
Committed: https://crrev.com/7f07ac7281819acd6eab53e28a2d45e425e2fd39
Cr-Commit-Position: refs/heads/master@{#314876}

[achaulk, 2015-01-30 18:12:09]

achaulk@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2015-02-02 19:21:53]

I don't think an negative index is possible - there's a similar range check in
kernel in input_handle_abs_event().

I'm thinking we should just make InProgressEvents an appropriately sized vector.

[spang, 2015-02-02 19:22:01]

https://codereview.chromium.org/891703004/diff/1/ui/events/ozone/evdev/touch_...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/891703004/diff/1/ui/events/ozone/evdev/touch_...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:174: input.code !=
ABS_MT_SLOT) {
Can't we keep the range check inside "case ABS_MT_SLOT"?

I don't see why we'd do it for every event. It should only be necessary when
updating current_slot_.

[achaulk, 2015-02-02 19:27:30]

On 2015/02/02 19:21:53, spang wrote:
> I don't think an negative index is possible - there's a similar range check in
> kernel in input_handle_abs_event().
> 
> I'm thinking we should just make InProgressEvents an appropriately sized
vector.

There are some crashes in bitset::set(), but we do check for indices > the
maximum before calling into the function, so the only possibility left is
negative indices

[spang, 2015-02-02 19:30:07]

On 2015/02/02 19:27:30, achaulk wrote:
> On 2015/02/02 19:21:53, spang wrote:
> > I don't think an negative index is possible - there's a similar range check
in
> > kernel in input_handle_abs_event().
> > 
> > I'm thinking we should just make InProgressEvents an appropriately sized
> vector.
> 
> There are some crashes in bitset::set(), but we do check for indices > the
> maximum before calling into the function, so the only possibility left is
> negative indices

We can add it to the range check but I think there's also something else wrong.

[achaulk, 2015-02-02 22:00:30]

Changed implementation to a vector

[spang, 2015-02-02 22:09:42]

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:116: 
I think you should create events_ here with size of 

  min(MAX_FINGERS, info.GetAbsMaximum(ABS_MT_SLOT) + 1)

No need to resize() later.

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:118: i <
std::min<int>(info.GetAbsMaximum(ABS_MT_SLOT) + 1, MAX_FINGERS);
needs adjustment to iterate the whole vector & initialize the new altered field

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:230:
events_[current_slot_].altered_ = true;
ABS_VOLUME or something like that shouldn't generate a touch event. So unless
you filter for ABS_MT ahead of time I think this was better before (set for
codes that changed MT state).

[achaulk, 2015-02-03 16:51:49]

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:116: 
On 2015/02/02 22:09:42, spang wrote:
> I think you should create events_ here with size of 
> 
>   min(MAX_FINGERS, info.GetAbsMaximum(ABS_MT_SLOT) + 1)
> 
> No need to resize() later.

Done.

https://codereview.chromium.org/891703004/diff/20001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:230:
events_[current_slot_].altered_ = true;
On 2015/02/02 22:09:42, spang wrote:
> ABS_VOLUME or something like that shouldn't generate a touch event. So unless
> you filter for ABS_MT ahead of time I think this was better before (set for
> codes that changed MT state).

The default action returns early now

[spang, 2015-02-03 17:05:23]

just one more thing

https://codereview.chromium.org/891703004/diff/40001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/891703004/diff/40001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:253:
events_[current_slot_++].type_ = ET_TOUCH_PRESSED;
if current_slot_ == size() - 1 then ++ here will put it of range.

Maybe:

events_[current_slot_] = ET_TOUCH_PRESSED;
current_slot_ = min(current_slot_ + 1, event_.size() - 1);

[achaulk, 2015-02-04 16:40:34]

https://codereview.chromium.org/891703004/diff/40001/ui/events/ozone/evdev/to...
File ui/events/ozone/evdev/touch_event_converter_evdev.cc (right):

https://codereview.chromium.org/891703004/diff/40001/ui/events/ozone/evdev/to...
ui/events/ozone/evdev/touch_event_converter_evdev.cc:253:
events_[current_slot_++].type_ = ET_TOUCH_PRESSED;
On 2015/02/03 17:05:23, spang wrote:
> if current_slot_ == size() - 1 then ++ here will put it of range.
> 
> Maybe:
> 
> events_[current_slot_] = ET_TOUCH_PRESSED;
> current_slot_ = min(current_slot_ + 1, event_.size() - 1);

Oops, good catch

[spang, 2015-02-04 18:07:11]

lgtm

[achaulk, 2015-02-04 18:07:32]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-02-04 18:07:35]

The CQ bit was unchecked by achaulk@chromium.org

[achaulk, 2015-02-04 19:20:48]

New patchsets have been uploaded after l-g-t-m from spang@chromium.org

[achaulk, 2015-02-05 20:47:35]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2015-02-05 20:48:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891703004/120001

[commit-bot: I haz the power, 2015-02-05 21:14:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-05 21:15:18]

Patchset 7 (id:??) landed as
https://crrev.com/7f07ac7281819acd6eab53e28a2d45e425e2fd39
Cr-Commit-Position: refs/heads/master@{#314876}