[image-decoders] Convert BMP image reader to use safe buffer operations [chromium/src : main]
Ho Cheung (Gerrit)
Ho Cheung added 1 comment![]( "Open in Gerrit")
Related details
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 6 comments![]( "Open in Gerrit")
reader by migrating to modern C++ safe containers and operations.Thanks for the CL. I am not familiar with `base::span`. Do you know of a Chrome developer who could be the primary reviewer?
In this first round of review, I have reviewed bmp_image_reader.h (completely) and only the last change to bmp_image_reader.cc,
std::array<uint32_t, 4> bit_masks_;Is it also recommended to change a C array to `std::array`?
At least in this CL, this kind of change doesn't seem necessary, in the sense that it doesn't really make these arrays more secure.
// of the return value, the caller won't read it.Delete this comment. It explained why `pixel` wasn't initialized in the original code. The new code initializes `pixel` (to 0).
// It doesn't matter that we never set the most significant byteThis comment shows this code assumes the computer is little-endian. I found this assumption documented and checked in base/numerics/byte_conversions.h:
```
// Chromium only builds and runs on Little Endian machines.
static_assert(std::endian::native == std::endian::little);
```
// TODO(crbug.com/351564777): Remove this and convert code to safer constructs.Should we add this bug number to the `Bug:` line in the commit message?
SetI(color_indexes[which]);Here `color_indexes[which]` is safe because `which` can only take the values 0 and 1.
Is this kind of proof acceptable for removing the `UNSAFE_TODO` macro? Or do we need to add a `CHECK`?
```
+ CHECK_LT(which, color_indexes.size());
if (color_indexes[which] < color_table_.size()) {
SetI(color_indexes[which]);
```
Related details
- Ho Cheung
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. |
Ho Cheung (Gerrit)
Ho Cheung voted and added 6 comments![]( "Open in Gerrit")
Votes added by Ho Cheung
| Commit-Queue | +1 |
6 comments
+tkent
Hi, please take a look at this CL and all the discussion threads, thank you!
reader by migrating to modern C++ safe containers and operations.Thanks for the CL. I am not familiar with `base::span`. Do you know of a Chrome developer who could be the primary reviewer?
In this first round of review, I have reviewed bmp_image_reader.h (completely) and only the last change to bmp_image_reader.cc,
Thank you for your review, I will add Kent Tamura to the list of reviewers later.
Is it also recommended to change a C array to `std::array`?
At least in this CL, this kind of change doesn't seem necessary, in the sense that it doesn't really make these arrays more secure.
If we don't convert C-style arrays to std::array, the clang plugin will consider them unsafe. For more information, please see:
https://chromium.googlesource.com/chromium/src/+/main/docs/unsafe_buffers.md#use-of-std_array
Delete this comment. It explained why `pixel` wasn't initialized in the original code. The new code initializes `pixel` (to 0).
Done
// TODO(crbug.com/351564777): Remove this and convert code to safer constructs.Should we add this bug number to the `Bug:` line in the commit message?
As mentioned in issue 451652367,
```
This issue is a replacement for issue 351564777, which reached the update limit.
```
So I chose to only include 451652367 in the commit message.
Here `color_indexes[which]` is safe because `which` can only take the values 0 and 1.
Is this kind of proof acceptable for removing the `UNSAFE_TODO` macro? Or do we need to add a `CHECK`?
```
+ CHECK_LT(which, color_indexes.size());
if (color_indexes[which] < color_table_.size()) {
SetI(color_indexes[which]);
```
- Kent Tamura
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura added 2 comments![]( "Open in Gerrit")
// It doesn't matter that we never set the most significant byte
// of the return value, the caller won't read it.
uint32_t pixel;This comment is still helpful because it's weird to copy only 3 bytes into a uint32_t.
CHECK_LT(which, color_indexes.size());This check is redundant because our std::array::operator[] contains it.
Related details
- Ho Cheung
- Wan-Teh Chang
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. |
Ho Cheung (Gerrit)
Ho Cheung voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ho Cheung
| Commit-Queue | +1 |
3 comments
// It doesn't matter that we never set the most significant byte
// of the return value, the caller won't read it.
uint32_t pixel;This comment is still helpful because it's weird to copy only 3 bytes into a uint32_t.
Done
This check is redundant because our std::array::operator[] contains it.
- Kent Tamura
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted Code-Review+1![]( "Open in Gerrit")
- Ho Cheung
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ho Cheung (Gerrit)
Ho Cheung voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ho Cheung
| Commit-Queue | +2 |
1 comment
Thank you for your review, I will submit this patch and will be happy to code any subsequent changes if needed.
Related details
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[image-decoders] Convert BMP image reader to use safe buffer operations
This CL eliminates all unsafe buffer usage warnings in the BMP image
reader by migrating to modern C++ safe containers and operations.
- M third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.cc
- M third_party/blink/renderer/platform/image-decoders/bmp/bmp_image_reader.h
Code-Review: +1 by Kent Tamura
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
Ho CheungDelete this comment. It explained why `pixel` wasn't initialized in the original code. The new code initializes `pixel` (to 0).
Done
This comment wasn't deleted. Please write a new CL to delete this comment.
This comment explains why we don't need to initialize `pixel`. Since the new code initializes `pixel` (so the most significate byte of the return value is 0), the comment is out of date and should be deleted.
Related details
- Ho Cheung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wan-Teh Chang (Gerrit)
Wan-Teh Chang added 1 comment![]( "Open in Gerrit")
// of the return value, the caller won't read it.Ho CheungDelete this comment. It explained why `pixel` wasn't initialized in the original code. The new code initializes `pixel` (to 0).
Wan-Teh ChangDone
This comment wasn't deleted. Please write a new CL to delete this comment.
This comment explains why we don't need to initialize `pixel`. Since the new code initializes `pixel` (so the most significate byte of the return value is 0), the comment is out of date and should be deleted.
I see Kent suggested keeping this comment. Then we need to update this comment. For example,
```
// It doesn't matter that we set the most significant byte
// of the return value to 0, the caller won't read it.
```
Related details
- Ho Cheung
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ho Cheung (Gerrit)
Ho Cheung added 1 comment![]( "Open in Gerrit")
// of the return value, the caller won't read it.Ho CheungDelete this comment. It explained why `pixel` wasn't initialized in the original code. The new code initializes `pixel` (to 0).
Wan-Teh ChangDone
Wan-Teh ChangThis comment wasn't deleted. Please write a new CL to delete this comment.
This comment explains why we don't need to initialize `pixel`. Since the new code initializes `pixel` (so the most significate byte of the return value is 0), the comment is out of date and should be deleted.
I see Kent suggested keeping this comment. Then we need to update this comment. For example,
```
// It doesn't matter that we set the most significant byte
// of the return value to 0, the caller won't read it.
```
OK, I've submitted a new patch to address the new issue you raised.
https://chromium-review.googlesource.com/c/chromium/src/+/7046579
Related details
- Wan-Teh Chang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |