Fix unsafe buffer usage in gesture_interpreter_libevdev_cros.cc [chromium/src : main]
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
Thanks!
This looks good in general, except for some UNSAFE_TODO that I suspect can't be removed. Unfortunately, the try-jobs might have failed to cover this file.
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {I don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2Fsrc
I think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```
Maybe @hcu...@chromium.org knows what bots are testing this file.
Related details
- Aditi Page
- Harry Cutts
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harry Cutts (Gerrit)
Harry Cutts added 1 comment![]( "Open in Gerrit")
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {I don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2FsrcI think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```Maybe @hcu...@chromium.org knows what bots are testing this file.
I'm no longer on the ChromeOS input team, adding cros-p12s-reviews@ instead
Related details
- Aditi Page
- CrOS Platform Inputs Reviewers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Shadowed: hba...@chromium.org
Reviewer source(s):
hba...@chromium.org, kenalba is from owner(chrome/chromeos/input/config/OWNERS.evdev)
Related details
- Aditi Page
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {Harry CuttsI don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2FsrcI think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```Maybe @hcu...@chromium.org knows what bots are testing this file.
I'm no longer on the ChromeOS input team, adding cros-p12s-reviews@ instead
Yes, std::array sounds better for bound checking. I will make this change. Thanks!
Related details
- Arthur Sonzogni
- Harry Cutts
- Henry Barnor
- Kenneth Albanowski
Vincent Scheib (Gerrit)
Vincent Scheib added 1 comment![]( "Open in Gerrit")
key_state_diff[i] = new_key_state[i] ^ prev_key_state_[i];These are different arrays, one member, one passed in as parameter, and if code is to be locally certain it is same size bounds checks or sizes should be verified on this line.
Related details
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Scheib (Gerrit)
Vincent Scheib added 1 comment![]( "Open in Gerrit")
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {Harry CuttsI don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2FsrcI think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```Maybe @hcu...@chromium.org knows what bots are testing this file.
Aditi PageI'm no longer on the ChromeOS input team, adding cros-p12s-reviews@ instead
Yes, std::array sounds better for bound checking. I will make this change. Thanks!
@hcu...@chromium.org should you be removed from https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/OWNERS
?
Related details
- Aditi Page
- Arthur Sonzogni
- Harry Cutts
- Henry Barnor
- Kenneth Albanowski
Harry Cutts (Gerrit)
Harry Cutts added 1 comment![]( "Open in Gerrit")
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {Harry CuttsI don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2FsrcI think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```Maybe @hcu...@chromium.org knows what bots are testing this file.
Aditi PageI'm no longer on the ChromeOS input team, adding cros-p12s-reviews@ instead
Vincent ScheibYes, std::array sounds better for bound checking. I will make this change. Thanks!
@hcu...@chromium.org should you be removed from https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/OWNERS
?
Potentially, yes — I was kept on there initially since it was anticipated that I'd be doing cross-over work between ChromeOS and Android input, but all of that ended up being in platform/gestures/, which has its own OWNERS file anyway.
Related details
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
key_state_diff[i] = new_key_state[i] ^ prev_key_state_[i];These are different arrays, one member, one passed in as parameter, and if code is to be locally certain it is same size bounds checks or sizes should be verified on this line.
Sg! Made these changes here as well as in other places with the same issue.
Related details
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
key_state_diff[i] = new_key_state[i] ^ prev_key_state_[i];Aditi PageThese are different arrays, one member, one passed in as parameter, and if code is to be locally certain it is same size bounds checks or sizes should be verified on this line.
Sg! Made these changes here as well as in other places with the same issue.
Done
Vincent Scheib (Gerrit)
Vincent Scheib added 1 comment![]( "Open in Gerrit")
key_state_diff[i] = new_key_state[i] ^ prev_key_state_[i];Aditi PageThese are different arrays, one member, one passed in as parameter, and if code is to be locally certain it is same size bounds checks or sizes should be verified on this line.
Aditi PageSg! Made these changes here as well as in other places with the same issue.
Done
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
key_state_diff[i] = new_key_state[i] ^ prev_key_state_[i];Aditi PageThese are different arrays, one member, one passed in as parameter, and if code is to be locally certain it is same size bounds checks or sizes should be verified on this line.
Aditi PageSg! Made these changes here as well as in other places with the same issue.
Vincent ScheibDone
Thanks, the prev_key_state_ member as well?
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 9 comments![]( "Open in Gerrit")
Thanks for the cleanup! It looks like the try-jobs didn't cover this file, as there are a couple of compilation errors that slipped through. I also noted a spot where we can simplify the array copying logic. This is much cleaner code overall—happy to land this once those fixes are in.
It looks like the 4 bots I triggered were enough to find the compile error.
You can add in the commmit description:
```
Cq-Include-Trybots: luci.chromium.try:chromeos-amd64-generic-dbg
Cq-Include-Trybots: luci.chromium.try:chromeos-amd64-generic-rel
Cq-Include-Trybots: luci.chromium.try:chromeos-jacuzzi-rel
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel
```
to have them permanently added.
const std::array<const char*, kNumTimeBuckets> kClickDurationMetricNames = {{You renamed the global array `ClickDurationMetricNames` to `kClickDurationMetricNames`. So this won't compile.
You are in the unfortunate case where the CQ try-job aren't able to correctly determine which bots must run. I don't know myself, so I guess we can just proceed carefully and hope it won't get reverted. If it gets reverted, we would learn about the bot to use for the next attempt.
I added additional bots.
static constexpr std::array<unsigned int, 8> kModifierEvdevCodes = {
{KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA, KEY_LEFTCTRL,
KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT}};Optional, feel free to skip:
Sometimes, developers want to the size to be deduced. `std::to_array` can be used:
```
static constexpr auto kModifierEvdevCodes = std::to_array<unsigned int>({
KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA,
KEY_LEFTCTRL, KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT
});
```
Note that it is sometimes possible (not here) to deduce the type and the size class template argument deduction:
```
static constexpr std::array kModifierEvdevCodes = {
KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA,
KEY_LEFTCTRL, KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT
};
```
but this won't work here, because it wouldn't give `unsigned int`
if (EvdevBitIsSet(new_key_state, key)) {I don't think that compile given the implementation:
```
static inline bool EvdevBitIsSet(const unsigned long* data, int bit) {
return UNSAFE_TODO(data[bit / EVDEV_LONG_BITS]) &
(1UL << (bit % EVDEV_LONG_BITS));
}
```
I think you can use:
```
EvdevBitIsSet(new_key_state.data(), key)
```
Ideally, we would spanify `EvDevBitIsSet`, remove the UNSAFE_TODO and update all the callsite, but this is likely too much for now, so you can skip this part and let it for someone else in the future.
if (EvdevBitIsSet(key_state_diff, key)) {ditto
bool value = EvdevBitIsSet(new_key_state, key);ditto
for (unsigned long i = 0; i < EVDEV_BITS_TO_LONGS(KEY_CNT); ++i) {
prev_key_state_[i] = new_key_state[i];
}`std::array` has better developer ergonomics. You can simplify and use the copy operator:
```suggestion
prev_key_state_ = new_key_state;
```
unsigned long copy_key_state[EVDEV_BITS_TO_LONGS(KEY_CNT)];
static_assert(sizeof(copy_key_state) == sizeof(prev_key_state_));
std::copy(std::begin(prev_key_state_), std::end(prev_key_state_),
std::begin(copy_key_state));Since `std::array` has better developers ergonomic, we can use its copy constructor, simplify and remove the assertion.
(Much simpler)
```suggestion
auto copy_key_state = prev_key_state_;
```
Related details
- Aditi Page
Aditi Page (Gerrit)
Aditi Page added 9 comments![]( "Open in Gerrit")
It looks like the 4 bots I triggered were enough to find the compile error.
You can add in the commmit description:
```
Cq-Include-Trybots: luci.chromium.try:chromeos-amd64-generic-dbg
Cq-Include-Trybots: luci.chromium.try:chromeos-amd64-generic-rel
Cq-Include-Trybots: luci.chromium.try:chromeos-jacuzzi-rel
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel
```
to have them permanently added.
Acknowledged
const std::array<const char*, kNumTimeBuckets> kClickDurationMetricNames = {{You renamed the global array `ClickDurationMetricNames` to `kClickDurationMetricNames`. So this won't compile.
You are in the unfortunate case where the CQ try-job aren't able to correctly determine which bots must run. I don't know myself, so I guess we can just proceed carefully and hope it won't get reverted. If it gets reverted, we would learn about the bot to use for the next attempt.I added additional bots.
Done
if (duration <= UNSAFE_TODO(ClickDurationMetricBuckets[time_bucket])) {Harry CuttsI don't think this one can be removed.
It is defined as a C-style array without bound checking:
https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=50?q=ClickDurationMetricNames&sq=&ss=chromium%2Fchromium%2FsrcI think we should turn this into a `std::array`.
I am surprised the CQ try-job did not find it.
I added additional bots to potentially increase coverage.
```
Cq-Include-Trybots: luci.chromium.try:chromeos-octopus-rel;luci.chrome.try:chromeos-octopus-chrome,chromeos-octopus-compile-chrome
```Maybe @hcu...@chromium.org knows what bots are testing this file.
Aditi PageI'm no longer on the ChromeOS input team, adding cros-p12s-reviews@ instead
Vincent ScheibYes, std::array sounds better for bound checking. I will make this change. Thanks!
Harry Cutts@hcu...@chromium.org should you be removed from https://source.chromium.org/chromium/chromium/src/+/main:ui/events/ozone/OWNERS
?
Potentially, yes — I was kept on there initially since it was anticipated that I'd be doing cross-over work between ChromeOS and Android input, but all of that ended up being in platform/gestures/, which has its own OWNERS file anyway.
Acknowledged
static constexpr std::array<unsigned int, 8> kModifierEvdevCodes = {
{KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA, KEY_LEFTCTRL,
KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT}};Optional, feel free to skip:
Sometimes, developers want to the size to be deduced. `std::to_array` can be used:
```
static constexpr auto kModifierEvdevCodes = std::to_array<unsigned int>({
KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA,
KEY_LEFTCTRL, KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT
});
```Note that it is sometimes possible (not here) to deduce the type and the size class template argument deduction:
```
static constexpr std::array kModifierEvdevCodes = {
KEY_LEFTALT, KEY_RIGHTALT, KEY_LEFTMETA, KEY_RIGHTMETA,
KEY_LEFTCTRL, KEY_RIGHTCTRL, KEY_LEFTSHIFT, KEY_RIGHTSHIFT
};
```
but this won't work here, because it wouldn't give `unsigned int`
Got it. I figured this size won't change so it's safe to assign it to "8" here.
Thank you for the clarification.
if (EvdevBitIsSet(new_key_state, key)) {I don't think that compile given the implementation:
```
static inline bool EvdevBitIsSet(const unsigned long* data, int bit) {
return UNSAFE_TODO(data[bit / EVDEV_LONG_BITS]) &
(1UL << (bit % EVDEV_LONG_BITS));
}
```I think you can use:
```
EvdevBitIsSet(new_key_state.data(), key)
```Ideally, we would spanify `EvDevBitIsSet`, remove the UNSAFE_TODO and update all the callsite, but this is likely too much for now, so you can skip this part and let it for someone else in the future.
Done
if (EvdevBitIsSet(key_state_diff, key)) {Aditi Pageditto
Done
bool value = EvdevBitIsSet(new_key_state, key);Aditi Pageditto
Done
for (unsigned long i = 0; i < EVDEV_BITS_TO_LONGS(KEY_CNT); ++i) {
prev_key_state_[i] = new_key_state[i];
}`std::array` has better developer ergonomics. You can simplify and use the copy operator:
```suggestion
prev_key_state_ = new_key_state;
```
Done
unsigned long copy_key_state[EVDEV_BITS_TO_LONGS(KEY_CNT)];
static_assert(sizeof(copy_key_state) == sizeof(prev_key_state_));
std::copy(std::begin(prev_key_state_), std::end(prev_key_state_),
std::begin(copy_key_state));Since `std::array` has better developers ergonomic, we can use its copy constructor, simplify and remove the assertion.
(Much simpler)
```suggestion
auto copy_key_state = prev_key_state_;
```
- Arthur Sonzogni
- Harry Cutts
- Henry Barnor
- Kenneth Albanowski
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page removed Harry Cutts from this change![]( "Open in Gerrit")
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Scheib (Gerrit)
Vincent Scheib voted Code-Review+1![]( "Open in Gerrit")
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
Thanks! That a nice cleanup of UNSAFE_TODO and code.
Related details
- Aditi Page
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
2 comments
lgtm % optional nit
Related details
- Aditi Page
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
int time_bucket;- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Henry Barnor
- Kenneth Albanowski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page (Gerrit)
Aditi Page removed Kenneth Albanowski from this change![]( "Open in Gerrit")
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Mitsuru Oshima
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page removed Henry Barnor from this change![]( "Open in Gerrit")
- Aditi Page
- Arthur Sonzogni
- Mitsuru Oshima
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
#include "base/memory/raw_ptr.h"Add:
```
#include "base/containers/span.h"
```
Related details
- Aditi Page
- Mitsuru Oshima
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
#include "base/memory/raw_ptr.h"Aditi PageAdd:
```
#include "base/containers/span.h"
```
Acknowledged
Related details
- Arthur Sonzogni
- Mitsuru Oshima
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted Commit-Queue+1![]( "Open in Gerrit")
- Aditi Page
- Arthur Sonzogni
- Vincent Scheib
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
1 comment
+hbarnor@ who touched this file recently.
hbarnor@ could you please review this too?
aditipage@@, please wait for his approval.
Related details
- Aditi Page
- Arthur Sonzogni
- Henry Barnor
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
+hbarnor@ who touched this file recently.
hbarnor@ could you please review this too?
aditipage@@, please wait for his approval.