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.
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
stime_t timestamp) {Should you add:
CHECK_EQ(new_key_state.size(), EVDEV_BITS_TO_LONGS(KEY_CNT));
Or use base::span<T,N> to enforce the size is the expected one?
std::copy(new_key_state.begin(), new_key_state.end(),
prev_key_state_.begin());std::ranges::copy is preferred, because it checks the size of the two range match.
Related details
- Aditi Page
- Henry Barnor
- 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. |
Henry Barnor (Gerrit)
Henry Barnor added 1 comment![]( "Open in Gerrit")
Fixed: 455863798Looks like we have a race between this CL and [crrev/c/7257175](https://chromium-review.googlesource.com/c/chromium/src/+/7257175) thus the merge conflict.
Please rebase this CL since both CL's are making similar changes.
Related details
- Aditi Page
- Vincent Scheib
Aditi Page (Gerrit)
Aditi Page added 3 comments![]( "Open in Gerrit")
Looks like we have a race between this CL and [crrev/c/7257175](https://chromium-review.googlesource.com/c/chromium/src/+/7257175) thus the merge conflict.
Please rebase this CL since both CL's are making similar changes.
Acknowledged
stime_t timestamp) {Should you add:
CHECK_EQ(new_key_state.size(), EVDEV_BITS_TO_LONGS(KEY_CNT));Or use base::span<T,N> to enforce the size is the expected one?
Looks like this was fixed in the other CL mentioned by Henry: crrev/c/7257175
std::copy(new_key_state.begin(), new_key_state.end(),
prev_key_state_.begin());std::ranges::copy is preferred, because it checks the size of the two range match.
After rebasing, I see from this CL (base::span<T,N> to enforce the size), that base::span was used.
Related details
- Arthur Sonzogni
- Henry Barnor
- 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 2 comments![]( "Open in Gerrit")
stime_t timestamp) {Aditi PageShould you add:
CHECK_EQ(new_key_state.size(), EVDEV_BITS_TO_LONGS(KEY_CNT));Or use base::span<T,N> to enforce the size is the expected one?
Looks like this was fixed in the other CL mentioned by Henry: crrev/c/7257175
Done
std::copy(new_key_state.begin(), new_key_state.end(),
prev_key_state_.begin());Aditi Pagestd::ranges::copy is preferred, because it checks the size of the two range match.
After rebasing, I see from this CL (base::span<T,N> to enforce the size), that base::span was used.
Done
Related details
- Arthur Sonzogni
- Henry Barnor
- Mitsuru Oshima
- Vincent Scheib
Code-Coverage
Code-Owners
Code-Review
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Sounds great, but could you please fix the compile errors? E.G. removing `.data()`, since we are now passing a span to the function (which is better)
Related details
- Aditi Page
- Henry Barnor
- 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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
ping me when the build error is fixed.
const std::array<float, kNumTimeBuckets> ClickDurationMetricBuckets = {{constexpr ... kClickDuration ...
Related details
- Aditi Page
- Henry Barnor
- Vincent Scheib
Aditi Page (Gerrit)
Aditi Page voted and added 2 comments![]( "Open in Gerrit")
Votes added by Aditi Page
| Commit-Queue | +1 |
Sounds great, but could you please fix the compile errors? E.G. removing `.data()`, since we are now passing a span to the function (which is better)
Done
const std::array<float, kNumTimeBuckets> ClickDurationMetricBuckets = {{Aditi Pageconstexpr ... kClickDuration ...
Done
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 7 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
7 comments
equivalents (std::ranges::fill and std::copy) to fix unsafe bufferYou can remove this, as we don't use std::copy.
EvdevKeyState prev_key_state_;Could you please revert this? This sounds like a regression to me, and unrelated to this patch.
#include "base/containers/contains.h"unused
constexpr std::array<float, kNumTimeBuckets> kClickDurationMetricBuckets = {{
0.15,
0.16,
0.17,
0.18,
0.19,
0.2,
0.25,
0.3,
0.35,
0.45,
0.55,
0.65,
0.75,
}};nit: Double braces aren't necessary in c++20.
nit: maybe you can add trailing zero to align the numbers.
```
constexpr std::array<float, kNumTimeBuckets> kClickDurationMetricBuckets = {
0.15,
0.16,
0.17,
0.18,
0.19,
0.20,
0.25,
0.30,
0.35,
0.45,
0.55,
0.65,
0.75,
};
```
constexpr std::array<const char*, kNumTimeBuckets> kClickDurationMetricNames = {Ditto: removing double braces?
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. |
Henry Barnor (Gerrit)
Henry Barnor added 1 comment![]( "Open in Gerrit")
LGTM
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
EvdevKeyState prev_key_state_;Could you please revert this? This sounds like a regression to me, and unrelated to this patch.
- Aditi Page
- Vincent Scheib
Aditi Page (Gerrit)
Aditi Page voted and added 6 comments![]( "Open in Gerrit")
Votes added by Aditi Page
| Commit-Queue | +1 |
6 comments
equivalents (std::ranges::fill and std::copy) to fix unsafe bufferYou can remove this, as we don't use std::copy.
Done
EvdevKeyState prev_key_state_;Mitsuru OshimaCould you please revert this? This sounds like a regression to me, and unrelated to this patch.
+1
Done
#include "base/containers/contains.h"Aditi Pageunused
Done
#include "base/containers/span.h"Aditi Pageunused
Done
constexpr std::array<float, kNumTimeBuckets> kClickDurationMetricBuckets = {{
0.15,
0.16,
0.17,
0.18,
0.19,
0.2,
0.25,
0.3,
0.35,
0.45,
0.55,
0.65,
0.75,
}};nit: Double braces aren't necessary in c++20.
nit: maybe you can add trailing zero to align the numbers.
```
constexpr std::array<float, kNumTimeBuckets> kClickDurationMetricBuckets = {
0.15,
0.16,
0.17,
0.18,
0.19,
0.20,
0.25,
0.30,
0.35,
0.45,
0.55,
0.65,
0.75,
};
```
Done
constexpr std::array<const char*, kNumTimeBuckets> kClickDurationMetricNames = {Aditi PageDitto: removing double braces?
Done
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 3 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
3 comments
HardwareState hwstate{};This is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
std::ranges::fill(fingers, FingerState{});They are already initialized.
```
// `FixedArray` provides `absl::FixedArray` in Chromium, but when `T` is
// trivially-default-constructible, forces the no-default-value constructor to
// initialize the elements to `T()`, instead of leaving them uninitialized.
```
so you can just remove this line.
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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima added 2 comments![]( "Open in Gerrit")
HardwareState hwstate{};This is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
do we? I'm fine with "={}", but want to know where it is described. Do you have a link to the doc?
std::ranges::fill(fingers, FingerState{});They are already initialized.
```
// `FixedArray` provides `absl::FixedArray` in Chromium, but when `T` is
// trivially-default-constructible, forces the no-default-value constructor to
// initialize the elements to `T()`, instead of leaving them uninitialized.
```
so you can just remove this line.
- Aditi Page
- Vincent Scheib
Aditi Page (Gerrit)
Aditi Page added 2 comments![]( "Open in Gerrit")
HardwareState hwstate{};Mitsuru OshimaThis is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
do we? I'm fine with "={}", but want to know where it is described. Do you have a link to the doc?
I believe this is explained here: https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/88.md?cl=head
Mitsuru OshimaThey are already initialized.
```
// `FixedArray` provides `absl::FixedArray` in Chromium, but when `T` is
// trivially-default-constructible, forces the no-default-value constructor to
// initialize the elements to `T()`, instead of leaving them uninitialized.
```
so you can just remove this line.
+1
- Arthur Sonzogni
- 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. |
Mitsuru Oshima (Gerrit)
Mitsuru Oshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mitsuru Oshima
| Code-Review | +1 |
1 comment
HardwareState hwstate{};Mitsuru OshimaThis is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
Aditi Pagedo we? I'm fine with "={}", but want to know where it is described. Do you have a link to the doc?
I believe this is explained here: https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/88.md?cl=head
- Aditi Page
- Arthur Sonzogni
- 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 added 1 comment![]( "Open in Gerrit")
HardwareState hwstate{};Mitsuru OshimaThis is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
Aditi Pagedo we? I'm fine with "={}", but want to know where it is described. Do you have a link to the doc?
Mitsuru OshimaI believe this is explained here: https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/88.md?cl=head
Acknowledged
Thanks: Aditi, I knew dcheng sent this to me, but I couldn't retrieve it.
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Aditi Page
- 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")
HardwareState hwstate{};Mitsuru OshimaThis is good, but I believe we prefer:
```
HardwareState hwstate = {};
```
in Chromium.
Aditi Pagedo we? I'm fine with "={}", but want to know where it is described. Do you have a link to the doc?
I believe this is explained here: https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/88.md?cl=head
- Arthur Sonzogni
- Vincent Scheib
Aditi Page (Gerrit)
Aditi Page voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix unsafe buffer usage in gesture_interpreter_libevdev_cros.cc
Replaced memset and memcpy with safer C++ standard library equivalents
to fix unsafe buffer operations. Also removed unnecessary UNSAFE_TODO
annotations.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.py
- M ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc
Code-Review: +1 by Arthur Sonzogni, +1 by Mitsuru Oshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |