From 566c72cd46a509d5f18a5f35f6d420a5451d51c0 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Mon, 14 Oct 2024 03:46:24 -0500 Subject: [PATCH] fix: -Wunsafe-buffer-usage warning in ChunkedDataPipeReadableStream (#44211) * chore: rename v8_value_serializer.cc,h to v8_util.cc,h * feat: add electron::util::as_byte_span(v8::Local) * fix: -Wunsafe-buffer-usage warnings in ChunkedDataPipeReadableStream::ReadInternal() Xref: https://chromium-review.googlesource.com/c/chromium/src/+/5619253 * refactor: restore node buffer span util * refactor: remove redundant span wrapper --- filenames.gni | 4 ++-- shell/browser/api/electron_api_app.cc | 2 +- .../api/electron_api_utility_process.cc | 2 +- .../browser/api/electron_api_web_contents.cc | 2 +- .../api/electron_api_web_frame_main.cc | 2 +- shell/browser/api/message_port.cc | 2 +- .../electron_crypto_module_delegate_nss.cc | 2 +- .../common/gin_converters/blink_converter.cc | 2 +- shell/common/gin_converters/net_converter.cc | 6 ++---- .../{v8_value_serializer.cc => v8_util.cc} | 21 ++++++++++++++++++- .../{v8_value_serializer.h => v8_util.h} | 7 +++++++ .../renderer/api/electron_api_ipc_renderer.cc | 2 +- shell/renderer/electron_api_service_impl.cc | 2 +- shell/services/node/parent_port.cc | 2 +- 14 files changed, 41 insertions(+), 17 deletions(-) rename shell/common/{v8_value_serializer.cc => v8_util.cc} (92%) rename shell/common/{v8_value_serializer.h => v8_util.h} (86%) diff --git a/filenames.gni b/filenames.gni index 5bb446ae2d81..9418c85f83c6 100644 --- a/filenames.gni +++ b/filenames.gni @@ -684,8 +684,8 @@ filenames = { "shell/common/skia_util.cc", "shell/common/skia_util.h", "shell/common/thread_restrictions.h", - "shell/common/v8_value_serializer.cc", - "shell/common/v8_value_serializer.h", + "shell/common/v8_util.cc", + "shell/common/v8_util.h", "shell/common/world_ids.h", "shell/renderer/api/context_bridge/object_cache.cc", "shell/renderer/api/context_bridge/object_cache.h", diff --git a/shell/browser/api/electron_api_app.cc b/shell/browser/api/electron_api_app.cc index 4a2d21931a07..9aadb51b0263 100644 --- a/shell/browser/api/electron_api_app.cc +++ b/shell/browser/api/electron_api_app.cc @@ -80,7 +80,7 @@ #include "shell/common/node_includes.h" #include "shell/common/options_switches.h" #include "shell/common/thread_restrictions.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "ui/gfx/image/image.h" #if BUILDFLAG(IS_WIN) diff --git a/shell/browser/api/electron_api_utility_process.cc b/shell/browser/api/electron_api_utility_process.cc index ab813d7aba80..35bb3972958f 100644 --- a/shell/browser/api/electron_api_utility_process.cc +++ b/shell/browser/api/electron_api_utility_process.cc @@ -28,7 +28,7 @@ #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/object_template_builder.h" #include "shell/common/node_includes.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "third_party/blink/public/common/messaging/message_port_descriptor.h" #include "third_party/blink/public/common/messaging/transferable_message_mojom_traits.h" diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index a9a96e74e2a1..1cbd5502a653 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -135,7 +135,7 @@ #include "shell/common/node_util.h" #include "shell/common/options_switches.h" #include "shell/common/thread_restrictions.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "storage/browser/file_system/isolated_context.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/public/common/input/web_input_event.h" diff --git a/shell/browser/api/electron_api_web_frame_main.cc b/shell/browser/api/electron_api_web_frame_main.cc index 5b3b2644fcaa..d020a9ef7410 100644 --- a/shell/browser/api/electron_api_web_frame_main.cc +++ b/shell/browser/api/electron_api_web_frame_main.cc @@ -31,7 +31,7 @@ #include "shell/common/gin_helper/object_template_builder.h" #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" namespace { diff --git a/shell/browser/api/message_port.cc b/shell/browser/api/message_port.cc index 1c9f73aff815..d09c9f881354 100644 --- a/shell/browser/api/message_port.cc +++ b/shell/browser/api/message_port.cc @@ -20,7 +20,7 @@ #include "shell/common/gin_helper/error_thrower.h" #include "shell/common/gin_helper/event_emitter_caller.h" #include "shell/common/node_includes.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "third_party/blink/public/common/messaging/transferable_message.h" #include "third_party/blink/public/common/messaging/transferable_message_mojom_traits.h" #include "third_party/blink/public/mojom/messaging/transferable_message.mojom.h" diff --git a/shell/browser/electron_crypto_module_delegate_nss.cc b/shell/browser/electron_crypto_module_delegate_nss.cc index 6d9b55daaa1f..207ca1fadc4b 100644 --- a/shell/browser/electron_crypto_module_delegate_nss.cc +++ b/shell/browser/electron_crypto_module_delegate_nss.cc @@ -11,7 +11,7 @@ #include "shell/common/gin_converters/callback_converter.h" #include "shell/common/gin_helper/callback.h" #include "shell/common/gin_helper/dictionary.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" ElectronNSSCryptoModuleDelegate::ElectronNSSCryptoModuleDelegate( const net::HostPortPair& server) diff --git a/shell/common/gin_converters/blink_converter.cc b/shell/common/gin_converters/blink_converter.cc index 68a0951a699a..c0d2648e085e 100644 --- a/shell/common/gin_converters/blink_converter.cc +++ b/shell/common/gin_converters/blink_converter.cc @@ -21,7 +21,7 @@ #include "shell/common/gin_converters/value_converter.h" #include "shell/common/gin_helper/dictionary.h" #include "shell/common/keyboard_util.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "third_party/blink/public/common/context_menu_data/edit_flags.h" #include "third_party/blink/public/common/input/web_input_event.h" #include "third_party/blink/public/common/input/web_keyboard_event.h" diff --git a/shell/common/gin_converters/net_converter.cc b/shell/common/gin_converters/net_converter.cc index 5fdb730c0f55..e326cdd7c279 100644 --- a/shell/common/gin_converters/net_converter.cc +++ b/shell/common/gin_converters/net_converter.cc @@ -34,6 +34,7 @@ #include "shell/common/gin_converters/value_converter.h" #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" +#include "shell/common/v8_util.h" namespace gin { @@ -368,10 +369,7 @@ class ChunkedDataPipeReadableStream final num_bytes = *size_ - bytes_read_; MojoResult rv = data_pipe_->ReadData( MOJO_READ_DATA_FLAG_NONE, - base::span(static_cast(buf->Buffer()->Data()), - buf->ByteLength()) - .subspan(buf->ByteOffset(), num_bytes), - num_bytes); + electron::util::as_byte_span(buf).first(num_bytes), num_bytes); if (rv == MOJO_RESULT_OK) { bytes_read_ += num_bytes; // Not needed for correctness, but this allows the consumer to send the diff --git a/shell/common/v8_value_serializer.cc b/shell/common/v8_util.cc similarity index 92% rename from shell/common/v8_value_serializer.cc rename to shell/common/v8_util.cc index 915a6779ba1e..7ed0f0034039 100644 --- a/shell/common/v8_value_serializer.cc +++ b/shell/common/v8_util.cc @@ -2,7 +2,7 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include #include @@ -240,4 +240,23 @@ v8::Local DeserializeV8Value(v8::Isolate* isolate, return V8Deserializer(isolate, data).Deserialize(); } +namespace util { + +/** + * SAFETY: There is not yet any v8::ArrayBufferView API that passes the + * UNSAFE_BUFFER_USAGE test, so let's isolate the unsafe API here. + * + * Where possible, Electron should use spans returned here instead of + * |v8::ArrayBufferView::Buffer()->Data()|, + * |v8::ArrayBufferView::ByteOffset()|, + * |v8::ArrayBufferView::ByteLength()|. + */ +base::span as_byte_span(v8::Local val) { + uint8_t* data = + static_cast(val->Buffer()->Data()) + val->ByteOffset(); + const size_t size = val->ByteLength(); + return UNSAFE_BUFFERS(base::span{data, size}); +} + +} // namespace util } // namespace electron diff --git a/shell/common/v8_value_serializer.h b/shell/common/v8_util.h similarity index 86% rename from shell/common/v8_value_serializer.h rename to shell/common/v8_util.h index 958677581bd4..59ba4c633a34 100644 --- a/shell/common/v8_value_serializer.h +++ b/shell/common/v8_util.h @@ -9,6 +9,7 @@ #include "ui/gfx/image/image_skia_rep.h" namespace v8 { +class ArrayBufferView; class Isolate; template class Local; @@ -29,6 +30,12 @@ v8::Local DeserializeV8Value(v8::Isolate* isolate, v8::Local DeserializeV8Value(v8::Isolate* isolate, base::span data); +namespace util { + +[[nodiscard]] base::span as_byte_span( + v8::Local abv); + +} // namespace util } // namespace electron #endif // ELECTRON_SHELL_COMMON_V8_VALUE_SERIALIZER_H_ diff --git a/shell/renderer/api/electron_api_ipc_renderer.cc b/shell/renderer/api/electron_api_ipc_renderer.cc index 35dab4c4c540..a93fa6a3c645 100644 --- a/shell/renderer/api/electron_api_ipc_renderer.cc +++ b/shell/renderer/api/electron_api_ipc_renderer.cc @@ -19,7 +19,7 @@ #include "shell/common/gin_helper/promise.h" #include "shell/common/node_bindings.h" #include "shell/common/node_includes.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" #include "third_party/blink/public/web/web_local_frame.h" #include "third_party/blink/public/web/web_message_port_converter.h" diff --git a/shell/renderer/electron_api_service_impl.cc b/shell/renderer/electron_api_service_impl.cc index 207f5b7d8aec..1b48c4a9814a 100644 --- a/shell/renderer/electron_api_service_impl.cc +++ b/shell/renderer/electron_api_service_impl.cc @@ -19,7 +19,7 @@ #include "shell/common/node_includes.h" #include "shell/common/options_switches.h" #include "shell/common/thread_restrictions.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "shell/renderer/electron_render_frame_observer.h" #include "shell/renderer/renderer_client_base.h" #include "third_party/blink/public/mojom/frame/user_activation_notification_type.mojom-shared.h" diff --git a/shell/services/node/parent_port.cc b/shell/services/node/parent_port.cc index e62b2076b2fb..012a588474bd 100644 --- a/shell/services/node/parent_port.cc +++ b/shell/services/node/parent_port.cc @@ -15,7 +15,7 @@ #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/event_emitter_caller.h" #include "shell/common/node_includes.h" -#include "shell/common/v8_value_serializer.h" +#include "shell/common/v8_util.h" #include "third_party/blink/public/common/messaging/transferable_message_mojom_traits.h" namespace electron {