Fix unsafe buffer usage in dawn_platform.cc [chromium/src : main]
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal voted and added 3 comments![]( "Open in Gerrit")
Votes added by Dibyajyoti Pal
| Code-Review | +1 |
3 comments
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
base::byte_span_from_ref(result)
.first(sizeof(handle))
.copy_from(base::byte_span_from_ref(handle));I'll leave it to the OWNERs to realize if this can be done in a simpler way.
I'm not sure why we can't just do the following based on [1]. It seems like this change is JUST removing the `UNSAFE_TODO` instead of actually digging deep into why `memcpy` is being used and whether that can be replaced, but I don't have the historical context to make this decision, so rubber stamping this!
```
result = handle.dummy;
```
[1] https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/trace_event_impl.h;l=41
Related details
- Aditi Page
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")
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
Initially, I thought it is redundant as well. However, if we remove the following static assert, I see flakiness with the bots [likely trybot flakes (WebCodecs)]:
std::is_trivial_v<base::trace_event::TraceEventHandle> &&
std::is_standard_layout_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be a plain old data type for byte-wise copy.");
Since I saw similar flakiness in patchset 1 and 3, I decided to keep both asserts ensuring redundancy but a more thorough check. If there's another way to go about it, I can explore it further.
base::byte_span_from_ref(result)
.first(sizeof(handle))
.copy_from(base::byte_span_from_ref(handle));I'll leave it to the OWNERs to realize if this can be done in a simpler way.
I'm not sure why we can't just do the following based on [1]. It seems like this change is JUST removing the `UNSAFE_TODO` instead of actually digging deep into why `memcpy` is being used and whether that can be replaced, but I don't have the historical context to make this decision, so rubber stamping this!
```
result = handle.dummy;
```[1] https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/trace_event_impl.h;l=41
Since this work falls under the Spanification umbrella, my primary goal was to replace the raw memcpy and remove the UNSAFE_TODO by using modern, bounds-checked abstractions.
I definitely appreciate the suggestion to use result = handle.dummy; as it would be much simpler for the current definition in src/. Just out of curiosity: would a direct member assignment be robust enough across all build configurations?
Related details
- Dibyajyoti Pal
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. |
Dibyajyoti Pal (Gerrit)
Dibyajyoti Pal added 2 comments![]( "Open in Gerrit")
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");Aditi Page`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
Initially, I thought it is redundant as well. However, if we remove the following static assert, I see flakiness with the bots [likely trybot flakes (WebCodecs)]:
std::is_trivial_v<base::trace_event::TraceEventHandle> &&
std::is_standard_layout_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be a plain old data type for byte-wise copy.");Since I saw similar flakiness in patchset 1 and 3, I decided to keep both asserts ensuring redundancy but a more thorough check. If there's another way to go about it, I can explore it further.
Hmmm interesting, are we sure that the failure was because of this code, or was that just a fluke that the test passed in this code?
Looking at the [flaky test logs](https://ci.chromium.org/ui/p/chromium/builders/try/win_optional_gpu_tests_rel/198733/test-results?view=legacy&q=ID%3A%3A%2F%2Fchrome%2Ftest%5C%3Atelemetry_gpu_integration_test%21flat%3A%3A%23gpu_tests.webcodecs_integration_test.WebCodecsIntegrationTest.WebCodecs_MixedSourceEncoding_av01.0.04M.08_prefer-software+VHash%3A9e8f0240a873a463), it seems like the failure is happening during the destruction of `~CanvasResourceProvider`, which in turn causes `surface_` to be [destructed](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_provider.h;l=308;bpv=1;bpt=1?q=CanvasResourceProvider). The error is also a `MEMORY_ACCESS_VIOLATION`, which happens when a memory location cannot be accessed. Without more context, I don't know if that code is hit because of the changes, or if the flake is because of other reasons.
I'm not sure that adding the first `static_assert` here will fix that, unless there's some C++ magic happening that I'm unaware of, as the check is redundant. Both `static_asserts` will pass and fail for the same condition, and since `std::is_trivial_v` already checks for ` std::is_trivially_copyable_v` under the hood, so the redundancy might make it confusing [1].
base::byte_span_from_ref(result)
.first(sizeof(handle))
.copy_from(base::byte_span_from_ref(handle));Aditi PageI'll leave it to the OWNERs to realize if this can be done in a simpler way.
I'm not sure why we can't just do the following based on [1]. It seems like this change is JUST removing the `UNSAFE_TODO` instead of actually digging deep into why `memcpy` is being used and whether that can be replaced, but I don't have the historical context to make this decision, so rubber stamping this!
```
result = handle.dummy;
```[1] https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/trace_event_impl.h;l=41
Since this work falls under the Spanification umbrella, my primary goal was to replace the raw memcpy and remove the UNSAFE_TODO by using modern, bounds-checked abstractions.
I definitely appreciate the suggestion to use result = handle.dummy; as it would be much simpler for the current definition in src/. Just out of curiosity: would a direct member assignment be robust enough across all build configurations?
Gotcha, if spanification is the goal then this might make sense.
To answer your second question, I don't see why this would not work, atleast from a coding perspective, since we're moving a value of type `uint64_t` into another. I don't have more context than that here, so would refer to a code owner here for more information.
Related details
- Aditi Page
- Kai Ninomiya
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. |
Kai Ninomiya (Gerrit)
Kai Ninomiya voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kai Ninomiya
| Code-Review | +1 |
2 comments
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");Aditi Page`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
Dibyajyoti PalInitially, I thought it is redundant as well. However, if we remove the following static assert, I see flakiness with the bots [likely trybot flakes (WebCodecs)]:
std::is_trivial_v<base::trace_event::TraceEventHandle> &&
std::is_standard_layout_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be a plain old data type for byte-wise copy.");Since I saw similar flakiness in patchset 1 and 3, I decided to keep both asserts ensuring redundancy but a more thorough check. If there's another way to go about it, I can explore it further.
Hmmm interesting, are we sure that the failure was because of this code, or was that just a fluke that the test passed in this code?
Looking at the [flaky test logs](https://ci.chromium.org/ui/p/chromium/builders/try/win_optional_gpu_tests_rel/198733/test-results?view=legacy&q=ID%3A%3A%2F%2Fchrome%2Ftest%5C%3Atelemetry_gpu_integration_test%21flat%3A%3A%23gpu_tests.webcodecs_integration_test.WebCodecsIntegrationTest.WebCodecs_MixedSourceEncoding_av01.0.04M.08_prefer-software+VHash%3A9e8f0240a873a463), it seems like the failure is happening during the destruction of `~CanvasResourceProvider`, which in turn causes `surface_` to be [destructed](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_provider.h;l=308;bpv=1;bpt=1?q=CanvasResourceProvider). The error is also a `MEMORY_ACCESS_VIOLATION`, which happens when a memory location cannot be accessed. Without more context, I don't know if that code is hit because of the changes, or if the flake is because of other reasons.
I'm not sure that adding the first `static_assert` here will fix that, unless there's some C++ magic happening that I'm unaware of, as the check is redundant. Both `static_asserts` will pass and fail for the same condition, and since `std::is_trivial_v` already checks for ` std::is_trivially_copyable_v` under the hood, so the redundancy might make it confusing [1].
Agree, I don't think a `static_assert` shouldn't change anything at runtime (unless there is undefined behavior).
base::byte_span_from_ref(result)
.first(sizeof(handle))
.copy_from(base::byte_span_from_ref(handle));Aditi PageI'll leave it to the OWNERs to realize if this can be done in a simpler way.
I'm not sure why we can't just do the following based on [1]. It seems like this change is JUST removing the `UNSAFE_TODO` instead of actually digging deep into why `memcpy` is being used and whether that can be replaced, but I don't have the historical context to make this decision, so rubber stamping this!
```
result = handle.dummy;
```[1] https://source.chromium.org/chromium/chromium/src/+/main:base/trace_event/trace_event_impl.h;l=41
Dibyajyoti PalSince this work falls under the Spanification umbrella, my primary goal was to replace the raw memcpy and remove the UNSAFE_TODO by using modern, bounds-checked abstractions.
I definitely appreciate the suggestion to use result = handle.dummy; as it would be much simpler for the current definition in src/. Just out of curiosity: would a direct member assignment be robust enough across all build configurations?
Gotcha, if spanification is the goal then this might make sense.
To answer your second question, I don't see why this would not work, atleast from a coding perspective, since we're moving a value of type `uint64_t` into another. I don't have more context than that here, so would refer to a code owner here for more information.
TraceEventHandle was more nontrivial until recently:
https://chromium-review.googlesource.com/c/chromium/src/+/6163823/6/base/trace_event/trace_event_impl.h
and did this in order to serialize it into something non-Chromium libraries could represent. We copied the method from V8, Skia, and ANGLE: https://chromium-review.googlesource.com/c/chromium/src/+/1745494/comment/77dc7705_e7e5bcb1/
I'd love to simplify this, but that would need to be either done by or reviewed by that team. I have filed https://crbug.com/471224082. If the tracing team is in support of the idea, you might be interested to take it on - it will result in the removal of several `memcpy`s.
Related details
- Aditi Page
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")
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");Aditi Page`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
Dibyajyoti PalInitially, I thought it is redundant as well. However, if we remove the following static assert, I see flakiness with the bots [likely trybot flakes (WebCodecs)]:
std::is_trivial_v<base::trace_event::TraceEventHandle> &&
std::is_standard_layout_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be a plain old data type for byte-wise copy.");Since I saw similar flakiness in patchset 1 and 3, I decided to keep both asserts ensuring redundancy but a more thorough check. If there's another way to go about it, I can explore it further.
Kai NinomiyaHmmm interesting, are we sure that the failure was because of this code, or was that just a fluke that the test passed in this code?
Looking at the [flaky test logs](https://ci.chromium.org/ui/p/chromium/builders/try/win_optional_gpu_tests_rel/198733/test-results?view=legacy&q=ID%3A%3A%2F%2Fchrome%2Ftest%5C%3Atelemetry_gpu_integration_test%21flat%3A%3A%23gpu_tests.webcodecs_integration_test.WebCodecsIntegrationTest.WebCodecs_MixedSourceEncoding_av01.0.04M.08_prefer-software+VHash%3A9e8f0240a873a463), it seems like the failure is happening during the destruction of `~CanvasResourceProvider`, which in turn causes `surface_` to be [destructed](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_provider.h;l=308;bpv=1;bpt=1?q=CanvasResourceProvider). The error is also a `MEMORY_ACCESS_VIOLATION`, which happens when a memory location cannot be accessed. Without more context, I don't know if that code is hit because of the changes, or if the flake is because of other reasons.
I'm not sure that adding the first `static_assert` here will fix that, unless there's some C++ magic happening that I'm unaware of, as the check is redundant. Both `static_asserts` will pass and fail for the same condition, and since `std::is_trivial_v` already checks for ` std::is_trivially_copyable_v` under the hood, so the redundancy might make it confusing [1].
Agree, I don't think a `static_assert` shouldn't change anything at runtime (unless there is undefined behavior).
- Dibyajyoti Pal
- Kai Ninomiya
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")
static_assert(
std::is_trivially_copyable_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be trivially copyable to be used with "
"base::byte_span_from_ref");Aditi Page`std::is_trivial_v` already includes the check that the type is `trivially copyable` and has a `trivial default constructor`, so let's remove this extra `static_assert` on L186?
Dibyajyoti PalInitially, I thought it is redundant as well. However, if we remove the following static assert, I see flakiness with the bots [likely trybot flakes (WebCodecs)]:
std::is_trivial_v<base::trace_event::TraceEventHandle> &&
std::is_standard_layout_v<base::trace_event::TraceEventHandle>,
"TraceEventHandle must be a plain old data type for byte-wise copy.");Since I saw similar flakiness in patchset 1 and 3, I decided to keep both asserts ensuring redundancy but a more thorough check. If there's another way to go about it, I can explore it further.
Kai NinomiyaHmmm interesting, are we sure that the failure was because of this code, or was that just a fluke that the test passed in this code?
Looking at the [flaky test logs](https://ci.chromium.org/ui/p/chromium/builders/try/win_optional_gpu_tests_rel/198733/test-results?view=legacy&q=ID%3A%3A%2F%2Fchrome%2Ftest%5C%3Atelemetry_gpu_integration_test%21flat%3A%3A%23gpu_tests.webcodecs_integration_test.WebCodecsIntegrationTest.WebCodecs_MixedSourceEncoding_av01.0.04M.08_prefer-software+VHash%3A9e8f0240a873a463), it seems like the failure is happening during the destruction of `~CanvasResourceProvider`, which in turn causes `surface_` to be [destructed](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/canvas_resource_provider.h;l=308;bpv=1;bpt=1?q=CanvasResourceProvider). The error is also a `MEMORY_ACCESS_VIOLATION`, which happens when a memory location cannot be accessed. Without more context, I don't know if that code is hit because of the changes, or if the flake is because of other reasons.
I'm not sure that adding the first `static_assert` here will fix that, unless there's some C++ magic happening that I'm unaware of, as the check is redundant. Both `static_asserts` will pass and fail for the same condition, and since `std::is_trivial_v` already checks for ` std::is_trivially_copyable_v` under the hood, so the redundancy might make it confusing [1].
Aditi PageAgree, I don't think a `static_assert` shouldn't change anything at runtime (unless there is undefined behavior).
Done
I have removed the static assert on L186 due to its redundancy. I see that the tests failing in previous patchsets, though on different platforms, are reported/known flakes. after re-running the tests, they seem to pass and are definitely not related to this change. Thank you for looking at this.
Related details
- Dibyajyoti Pal
- Kai Ninomiya
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kai Ninomiya (Gerrit)
Kai Ninomiya voted Code-Review+1![]( "Open in Gerrit")
- Aditi Page
- Dibyajyoti Pal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |