Fix unsafe buffer usage in ui/display/mac/screen_mac.mm [chromium/src : main]
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
This isn't any better than the lhs, the warning just happens to not flag it. tsepez, kinuko, do you know if we intentionally don't treat accesses off `vector::data()` as tained? `my_vec.data()[3]` isn't any better than `my_pointer[3]`, since it also loses the check that `my_vec[3]` does.
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 2 comments![]( "Open in Gerrit")
This isn't any better than the lhs, the warning just happens to not flag it. tsepez, kinuko, do you know if we intentionally don't treat accesses off `vector::data()` as tained? `my_vec.data()[3]` isn't any better than `my_pointer[3]`, since it also loses the check that `my_vec[3]` does.
Vector.data isn't tainted by itself. But when you write vector.data()[3], the application of [3] is onto a C-pointer and that should be flagged. Here we are passing data as a ptr, so the callee would be the place the unsafety is flagged.
Not sure this actually fixes any UNSAFE issues, either side should compile equivalently wrt. UNSAFE
std::vector<CGDirectDisplayID> online_displays(1024);this should be std::array<..., 1024>
Related details
- Arthur Sonzogni
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Tom SepezThis isn't any better than the lhs, the warning just happens to not flag it. tsepez, kinuko, do you know if we intentionally don't treat accesses off `vector::data()` as tained? `my_vec.data()[3]` isn't any better than `my_pointer[3]`, since it also loses the check that `my_vec[3]` does.
Vector.data isn't tainted by itself. But when you write vector.data()[3], the application of [3] is onto a C-pointer and that should be flagged. Here we are passing data as a ptr, so the callee would be the place the unsafety is flagged.
Not sure this actually fixes any UNSAFE issues, either side should compile equivalently wrt. UNSAFE
The callee is an SDK function though, so we never see its code.
Why is `vector.data()` safe but `stack_array` isn't?
Related details
- Arthur Sonzogni
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Tom SepezThis isn't any better than the lhs, the warning just happens to not flag it. tsepez, kinuko, do you know if we intentionally don't treat accesses off `vector::data()` as tained? `my_vec.data()[3]` isn't any better than `my_pointer[3]`, since it also loses the check that `my_vec[3]` does.
Nico WeberVector.data isn't tainted by itself. But when you write vector.data()[3], the application of [3] is onto a C-pointer and that should be flagged. Here we are passing data as a ptr, so the callee would be the place the unsafety is flagged.
Not sure this actually fixes any UNSAFE issues, either side should compile equivalently wrt. UNSAFE
The callee is an SDK function though, so we never see its code.
Why is `vector.data()` safe but `stack_array` isn't?
- Arthur Sonzogni
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Tom SepezThis isn't any better than the lhs, the warning just happens to not flag it. tsepez, kinuko, do you know if we intentionally don't treat accesses off `vector::data()` as tained? `my_vec.data()[3]` isn't any better than `my_pointer[3]`, since it also loses the check that `my_vec[3]` does.
Nico WeberVector.data isn't tainted by itself. But when you write vector.data()[3], the application of [3] is onto a C-pointer and that should be flagged. Here we are passing data as a ptr, so the callee would be the place the unsafety is flagged.
Not sure this actually fixes any UNSAFE issues, either side should compile equivalently wrt. UNSAFE
Tom SepezThe callee is an SDK function though, so we never see its code.
Why is `vector.data()` safe but `stack_array` isn't?
stack_array shouldn't be flagged by the compiler here.
This may just be the bot being wrong about what to change. I'd like to see the build error from the original code when the pragma is removed.
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
CGDirectDisplayID online_display = online_displays[online_display_index];Here is likely where things get flagged.
Nico Weber (Gerrit)
Nico Weber added 2 comments![]( "Open in Gerrit")
Oh derp, that makes sense. Sorry for the noise!
CGDirectDisplayID online_display = online_displays[online_display_index];Here is likely where things get flagged.
Yep:
```
../../ui/display/mac/screen_mac.mm:235:40: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
235 | CGDirectDisplayID online_display = online_displays[online_display_index];
| ^~~~~~~~~~~~~~~
```
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 2 comments![]( "Open in Gerrit")
this should be std::array<..., 1024>
Done
CGDirectDisplayID online_display = online_displays[online_display_index];Nico WeberHere is likely where things get flagged.
Yep:
```
../../ui/display/mac/screen_mac.mm:235:40: error: unsafe buffer access [-Werror,-Wunsafe-buffer-usage]
235 | CGDirectDisplayID online_display = online_displays[online_display_index];
| ^~~~~~~~~~~~~~~
```
- Arthur Sonzogni
- Nico Weber
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 3 comments![]( "Open in Gerrit")
This CL fixes an unsafe buffer usage in `ui/display/mac/screen_mac.mm` by replacing a C-style array with a `std::vector`.nit: std::array
CHECK_LE(static_cast<size_t>(online_display_count), online_displays.size());redundant, base::span<>::first() will trap on OOB so you don't have to.
Related details
- Arthur Sonzogni
- Daniel Soromou
- Nico Weber
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. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
.first(static_cast<size_t>(online_display_count));cast not required. GDisplayCount is (deprecated) alias for uin32_t, and on all platforms (32 or 64 bit) that promotes to size_t. If that should change, (i.e. CGDisplayCount became signed), then we'd catch this at compile time except for the cast which suppresses it.
Daniel Soromou (Gerrit)
Daniel Soromou added 3 comments![]( "Open in Gerrit")
This CL fixes an unsafe buffer usage in `ui/display/mac/screen_mac.mm` by replacing a C-style array with a `std::vector`.Daniel Soromounit: std::array
Done
CHECK_LE(static_cast<size_t>(online_display_count), online_displays.size());redundant, base::span<>::first() will trap on OOB so you don't have to.
Done
.first(static_cast<size_t>(online_display_count));cast not required. GDisplayCount is (deprecated) alias for uin32_t, and on all platforms (32 or 64 bit) that promotes to size_t. If that should change, (i.e. CGDisplayCount became signed), then we'd catch this at compile time except for the cast which suppresses it.
- Arthur Sonzogni
- Nico Weber
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 1 comment![]( "Open in Gerrit")
- Arthur Sonzogni
- Colin Blundell
- Daniel Soromou
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arthur Sonzogni
- Colin Blundell
- Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |