Browse Source

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<v8::ArrayBuffer>)

* 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
Charles Kerr 6 months ago
parent
commit
566c72cd46

+ 2 - 2
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",

+ 1 - 1
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)

+ 1 - 1
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"
 

+ 1 - 1
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"

+ 1 - 1
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 {
 

+ 1 - 1
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"

+ 1 - 1
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)

+ 1 - 1
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"

+ 2 - 4
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<uint8_t*>(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

+ 20 - 1
shell/common/v8_value_serializer.cc → 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 <utility>
 #include <vector>
@@ -240,4 +240,23 @@ v8::Local<v8::Value> 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<uint8_t> as_byte_span(v8::Local<v8::ArrayBufferView> val) {
+  uint8_t* data =
+      static_cast<uint8_t*>(val->Buffer()->Data()) + val->ByteOffset();
+  const size_t size = val->ByteLength();
+  return UNSAFE_BUFFERS(base::span{data, size});
+}
+
+}  // namespace util
 }  // namespace electron

+ 7 - 0
shell/common/v8_value_serializer.h → 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 T>
 class Local;
@@ -29,6 +30,12 @@ v8::Local<v8::Value> DeserializeV8Value(v8::Isolate* isolate,
 v8::Local<v8::Value> DeserializeV8Value(v8::Isolate* isolate,
                                         base::span<const uint8_t> data);
 
+namespace util {
+
+[[nodiscard]] base::span<uint8_t> as_byte_span(
+    v8::Local<v8::ArrayBufferView> abv);
+
+}  // namespace util
 }  // namespace electron
 
 #endif  // ELECTRON_SHELL_COMMON_V8_VALUE_SERIALIZER_H_

+ 1 - 1
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"

+ 1 - 1
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"

+ 1 - 1
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 {