Remove unsafe buffer patterns in display/win and gl_surface_egl [chromium/src : main]
Lloyd Huang (Gerrit)
Lloyd Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lloyd Huang
| Commit-Queue | +0 |
1 comment
Related details
- Mitsuru Oshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lloyd Huang (Gerrit)
Lloyd Huang added 1 comment![]( "Open in Gerrit")
Hi Mitsuru. PTAL.
With this CL and several recent related CLs, most of the UNSAFE_TODO
usages in ui/ that were practical to convert on Windows non-test code have
now been addressed.
The remaining UNSAFE_TODO cleanups are primarily in macOS- and
Linux-related codepaths and in test code, and I plan to handle those in
follow-up CLs.
Related details
- Mitsuru Oshima
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 added 1 comment![]( "Open in Gerrit")
- Kurt Catti-Schmidt
- Lloyd Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 2 comments![]( "Open in Gerrit")
absl::Cleanup release_scoped_array(
[&scoped_array]() { (void)scoped_array.Release(); });This shouldn't be necessary - the `ScopedSafearray` destructor will handle releasing the array
for (size_t i = 1; i < tracePairs.size(); i++) {Nit: Google C++ coding guidelines recommend `++i`: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
(I realize this was already there before this change, but you might as well fix it)
Related details
- Lloyd Huang
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. |
Lloyd Huang (Gerrit)
Lloyd Huang added 1 comment![]( "Open in Gerrit")
absl::Cleanup release_scoped_array(
[&scoped_array]() { (void)scoped_array.Release(); });This shouldn't be necessary - the `ScopedSafearray` destructor will handle releasing the array
`Release()` here is not an eager free. `V_ARRAY(block_content.ptr())` gives a SAFEARRAY that is still owned by `block_content`; `ScopedSafearray` is only being used as a temporary RAII wrapper so we can call `CreateLockScope()` safely.
The explicit `Release()` only drops `ScopedSafearray` ownership before its destructor runs. The actual destruction still happens later when `block_content` is cleared.
Without the manual `Release()`, `scoped_array` would call `SafeArrayDestroy()` in its destructor and leave `block_content` holding a dangling `parray`. When `block_content` is then destroyed, `VariantClear()` would try to destroy the same SAFEARRAY again. In practice that is a double-destroy.
Related details
- Kurt Catti-Schmidt
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. |
Lloyd Huang (Gerrit)
Lloyd Huang added 1 comment![]( "Open in Gerrit")
Nit: Google C++ coding guidelines recommend `++i`: https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement
(I realize this was already there before this change, but you might as well fix it)
Done
Related details
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
absl::Cleanup release_scoped_array(
[&scoped_array]() { (void)scoped_array.Release(); });Lloyd HuangThis shouldn't be necessary - the `ScopedSafearray` destructor will handle releasing the array
`Release()` here is not an eager free. `V_ARRAY(block_content.ptr())` gives a SAFEARRAY that is still owned by `block_content`; `ScopedSafearray` is only being used as a temporary RAII wrapper so we can call `CreateLockScope()` safely.
The explicit `Release()` only drops `ScopedSafearray` ownership before its destructor runs. The actual destruction still happens later when `block_content` is cleared.
Without the manual `Release()`, `scoped_array` would call `SafeArrayDestroy()` in its destructor and leave `block_content` holding a dangling `parray`. When `block_content` is then destroyed, `VariantClear()` would try to destroy the same SAFEARRAY again. In practice that is a double-destroy.
Can this `base::win::ScopedSafearray` be moved up to replace `SAFEARRAY* array` entirely, and would that remove the need for the manual release?
Related details
- Lloyd Huang
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. |
Lloyd Huang (Gerrit)
Lloyd Huang added 1 comment![]( "Open in Gerrit")
absl::Cleanup release_scoped_array(
[&scoped_array]() { (void)scoped_array.Release(); });Lloyd HuangThis shouldn't be necessary - the `ScopedSafearray` destructor will handle releasing the array
Kurt Catti-Schmidt`Release()` here is not an eager free. `V_ARRAY(block_content.ptr())` gives a SAFEARRAY that is still owned by `block_content`; `ScopedSafearray` is only being used as a temporary RAII wrapper so we can call `CreateLockScope()` safely.
The explicit `Release()` only drops `ScopedSafearray` ownership before its destructor runs. The actual destruction still happens later when `block_content` is cleared.
Without the manual `Release()`, `scoped_array` would call `SafeArrayDestroy()` in its destructor and leave `block_content` holding a dangling `parray`. When `block_content` is then destroyed, `VariantClear()` would try to destroy the same SAFEARRAY again. In practice that is a double-destroy.
Can this `base::win::ScopedSafearray` be moved up to replace `SAFEARRAY* array` entirely, and would that remove the need for the manual release?
Updated to move ownership of `BlockContent`’s `SAFEARRAY` into `base::win::ScopedSafearray` via `block_content.Release().parray`.
That removes the temporary dual-ownership pattern and the manual `Release()` cleanup.
Related details
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
1 comment
Latest version LGTM, thank you! You'll still need a review from @osh...@chromium.org for OWNERS approval.
Related details
- Lloyd Huang
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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Lloyd Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lloyd Huang (Gerrit)
Lloyd Huang voted Commit-Queue+2![]( "Open in Gerrit")
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
Remove unsafe buffer patterns in display/win and gl_surface_egl
Replace raw SAFEARRAY access in audio_edid_scan.cc with
ScopedSafearray/LockScope, value-initialize MONITORINFOEX in
color_profile_reader.cc, and switch the pending trace symbol handling in
gl_surface_egl.cc to std::string_view.
- M ui/display/win/audio_edid_scan.cc
- M ui/display/win/color_profile_reader.cc
- M ui/gl/gl_surface_egl.cc
Code-Review: +1 by Mitsuru Oshima, +1 by Kurt Catti-Schmidt
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |