From 8d7001adc358691ee249b9855fefd206d5f558d8 Mon Sep 17 00:00:00 2001 From: Nik Pavlov Date: Mon, 16 Oct 2023 22:43:29 +0000 Subject: [PATCH] Replace ReadOnlySharedMemoryRegion with WritableSharedMemoryRegion (see #3502) Write access to the shared memory region is required because JavaScript lacks the capability to create read-only ArrayBuffers. When a user attempts to modify an ArrayBuffer that utilizes a ReadOnlySharedMemoryRegion as its BackingStore it triggers an access violation. Note that this pull request may be reverted in the future if JavaScript adds read-only ArrayBuffer support. --- include/capi/cef_shared_memory_region_capi.h | 4 +-- include/cef_api_hash.h | 8 ++--- include/cef_shared_memory_region.h | 2 +- libcef/browser/browser_frame.cc | 2 +- libcef/browser/browser_frame.h | 2 +- libcef/browser/frame_host_impl.cc | 4 +-- libcef/browser/frame_host_impl.h | 2 +- libcef/common/mojom/cef.mojom | 4 +-- libcef/common/process_message_smr_impl.cc | 24 +++++++------ libcef/common/process_message_smr_impl.h | 11 +++--- libcef/renderer/frame_impl.cc | 4 +-- libcef/renderer/frame_impl.h | 2 +- .../cpptoc/shared_memory_region_cpptoc.cc | 8 ++--- .../ctocpp/shared_memory_region_ctocpp.cc | 8 ++--- .../ctocpp/shared_memory_region_ctocpp.h | 4 +-- .../shared_process_message_unittest.cc | 34 +++++++++++++++++++ 16 files changed, 81 insertions(+), 42 deletions(-) diff --git a/include/capi/cef_shared_memory_region_capi.h b/include/capi/cef_shared_memory_region_capi.h index 07be55e69..fbc131282 100644 --- a/include/capi/cef_shared_memory_region_capi.h +++ b/include/capi/cef_shared_memory_region_capi.h @@ -33,7 +33,7 @@ // by hand. See the translator.README.txt file in the tools directory for // more information. // -// $hash=08f64795d78bdad29a45222a7263e795ce77a52d$ +// $hash=dfa2f2d57339e05592d7ee5f4c4c54dd0932cd94$ // #ifndef CEF_INCLUDE_CAPI_CEF_SHARED_MEMORY_REGION_CAPI_H_ @@ -69,7 +69,7 @@ typedef struct _cef_shared_memory_region_t { /// Returns the pointer to the memory. Returns nullptr for invalid instances. /// The returned pointer is only valid for the life span of this object. /// - const void*(CEF_CALLBACK* memory)(struct _cef_shared_memory_region_t* self); + void*(CEF_CALLBACK* memory)(struct _cef_shared_memory_region_t* self); } cef_shared_memory_region_t; #ifdef __cplusplus diff --git a/include/cef_api_hash.h b/include/cef_api_hash.h index 024b8ed5e..7f1dee384 100644 --- a/include/cef_api_hash.h +++ b/include/cef_api_hash.h @@ -42,13 +42,13 @@ // way that may cause binary incompatibility with other builds. The universal // hash value will change if any platform is affected whereas the platform hash // values will change only if that particular platform is affected. -#define CEF_API_HASH_UNIVERSAL "f8f0a0431b4cf46f98e3e58af93199f21d403d69" +#define CEF_API_HASH_UNIVERSAL "e5e3eef669443880f747ebccf04ca470306acec9" #if defined(OS_WIN) -#define CEF_API_HASH_PLATFORM "e5142124a2c423c0c438909bbfb86cd35a2b839d" +#define CEF_API_HASH_PLATFORM "2a84aaf4b26c613183b48b4aac39aa57045287af" #elif defined(OS_MAC) -#define CEF_API_HASH_PLATFORM "5748f4bc71b501c804ef284e091e6eea847086d9" +#define CEF_API_HASH_PLATFORM "6e409fac3d2b069c084bc17a04196d4a93d5c028" #elif defined(OS_LINUX) -#define CEF_API_HASH_PLATFORM "23413591633deaf08aae5248becdb0118b105de1" +#define CEF_API_HASH_PLATFORM "18d2e48f53384ccb092b7dd4f0ae5dceafdac1fa" #endif #ifdef __cplusplus diff --git a/include/cef_shared_memory_region.h b/include/cef_shared_memory_region.h index ba708002c..126a9796c 100644 --- a/include/cef_shared_memory_region.h +++ b/include/cef_shared_memory_region.h @@ -63,7 +63,7 @@ class CefSharedMemoryRegion : public virtual CefBaseRefCounted { /// The returned pointer is only valid for the life span of this object. /// /*--cef()--*/ - virtual const void* Memory() = 0; + virtual void* Memory() = 0; }; #endif // CEF_INCLUDE_CEF_SHARED_MEMORY_REGION_H_ diff --git a/libcef/browser/browser_frame.cc b/libcef/browser/browser_frame.cc index 35ee61cd5..9b354d615 100644 --- a/libcef/browser/browser_frame.cc +++ b/libcef/browser/browser_frame.cc @@ -44,7 +44,7 @@ void CefBrowserFrame::SendMessage(const std::string& name, void CefBrowserFrame::SendSharedMemoryRegion( const std::string& name, - base::ReadOnlySharedMemoryRegion region) { + base::WritableSharedMemoryRegion region) { // Always send to the newly created RFH, which may be speculative when // navigating cross-origin. if (auto host = GetFrameHost(/*prefer_speculative=*/true)) { diff --git a/libcef/browser/browser_frame.h b/libcef/browser/browser_frame.h index 41576a09d..a9bf83c99 100644 --- a/libcef/browser/browser_frame.h +++ b/libcef/browser/browser_frame.h @@ -38,7 +38,7 @@ class CefBrowserFrame void SendMessage(const std::string& name, base::Value::List arguments) override; void SendSharedMemoryRegion(const std::string& name, - base::ReadOnlySharedMemoryRegion region) override; + base::WritableSharedMemoryRegion region) override; void FrameAttached(mojo::PendingRemote render_frame, bool reattached) override; void UpdateDraggableRegions( diff --git a/libcef/browser/frame_host_impl.cc b/libcef/browser/frame_host_impl.cc index b813bd2d3..bb9767fb5 100644 --- a/libcef/browser/frame_host_impl.cc +++ b/libcef/browser/frame_host_impl.cc @@ -289,7 +289,7 @@ void CefFrameHostImpl::SendProcessMessage( SendToRenderFrame( __FUNCTION__, base::BindOnce( - [](const CefString& name, base::ReadOnlySharedMemoryRegion region, + [](const CefString& name, base::WritableSharedMemoryRegion region, const RenderFrameType& render_frame) { render_frame->SendSharedMemoryRegion(name, std::move(region)); }, @@ -644,7 +644,7 @@ void CefFrameHostImpl::SendMessage(const std::string& name, void CefFrameHostImpl::SendSharedMemoryRegion( const std::string& name, - base::ReadOnlySharedMemoryRegion region) { + base::WritableSharedMemoryRegion region) { if (auto browser = GetBrowserHostBase()) { if (auto client = browser->GetClient()) { CefRefPtr message( diff --git a/libcef/browser/frame_host_impl.h b/libcef/browser/frame_host_impl.h index 832f5a40b..c61df87a8 100644 --- a/libcef/browser/frame_host_impl.h +++ b/libcef/browser/frame_host_impl.h @@ -138,7 +138,7 @@ class CefFrameHostImpl : public CefFrame, public cef::mojom::BrowserFrame { void SendMessage(const std::string& name, base::Value::List arguments) override; void SendSharedMemoryRegion(const std::string& name, - base::ReadOnlySharedMemoryRegion region) override; + base::WritableSharedMemoryRegion region) override; void FrameAttached(mojo::PendingRemote render_frame, bool reattached) override; void UpdateDraggableRegions( diff --git a/libcef/common/mojom/cef.mojom b/libcef/common/mojom/cef.mojom index ee6fb087f..fa2d0d481 100644 --- a/libcef/common/mojom/cef.mojom +++ b/libcef/common/mojom/cef.mojom @@ -59,7 +59,7 @@ interface RenderFrame { SendMessage(string name, mojo_base.mojom.ListValue arguments); // Send a shared memory region to the render process. - SendSharedMemoryRegion(string name, mojo_base.mojom.ReadOnlySharedMemoryRegion region); + SendSharedMemoryRegion(string name, mojo_base.mojom.WritableSharedMemoryRegion region); // Send a command. SendCommand(string command); @@ -91,7 +91,7 @@ interface BrowserFrame { SendMessage(string name, mojo_base.mojom.ListValue arguments); // Send a shared memory region to the browser process. - SendSharedMemoryRegion(string name, mojo_base.mojom.ReadOnlySharedMemoryRegion region); + SendSharedMemoryRegion(string name, mojo_base.mojom.WritableSharedMemoryRegion region); // The render frame is ready to begin handling actions. FrameAttached(pending_remote render_frame, diff --git a/libcef/common/process_message_smr_impl.cc b/libcef/common/process_message_smr_impl.cc index 4e04eb57c..922b272d1 100644 --- a/libcef/common/process_message_smr_impl.cc +++ b/libcef/common/process_message_smr_impl.cc @@ -12,7 +12,7 @@ namespace { class CefSharedMemoryRegionImpl final : public CefSharedMemoryRegion { public: - CefSharedMemoryRegionImpl(base::ReadOnlySharedMemoryMapping&& mapping) + CefSharedMemoryRegionImpl(base::WritableSharedMemoryMapping&& mapping) : mapping_(std::move(mapping)) {} CefSharedMemoryRegionImpl(const CefSharedMemoryRegionImpl&) = delete; CefSharedMemoryRegionImpl& operator=(const CefSharedMemoryRegionImpl&) = @@ -21,10 +21,10 @@ class CefSharedMemoryRegionImpl final : public CefSharedMemoryRegion { // CefSharedMemoryRegion methods bool IsValid() override { return mapping_.IsValid(); } size_t Size() override { return IsValid() ? mapping_.size() : 0; } - const void* Memory() override { return mapping_.memory(); } + void* Memory() override { return mapping_.memory(); } private: - base::ReadOnlySharedMemoryMapping mapping_; + base::WritableSharedMemoryMapping mapping_; IMPLEMENT_REFCOUNTING(CefSharedMemoryRegionImpl); }; @@ -32,7 +32,7 @@ class CefSharedMemoryRegionImpl final : public CefSharedMemoryRegion { CefProcessMessageSMRImpl::CefProcessMessageSMRImpl( const CefString& name, - base::ReadOnlySharedMemoryRegion&& region) + base::WritableSharedMemoryRegion&& region) : name_(name), region_(std::move(region)) { DCHECK(!name_.empty()); DCHECK(region_.IsValid()); @@ -53,7 +53,7 @@ CefProcessMessageSMRImpl::GetSharedMemoryRegion() { return new CefSharedMemoryRegionImpl(region_.Map()); } -base::ReadOnlySharedMemoryRegion CefProcessMessageSMRImpl::TakeRegion() { +base::WritableSharedMemoryRegion CefProcessMessageSMRImpl::TakeRegion() { return std::move(region_); } @@ -68,23 +68,27 @@ CefSharedProcessMessageBuilderImpl::CefSharedProcessMessageBuilderImpl( const CefString& name, size_t byte_size) : name_(name), - region_(base::ReadOnlySharedMemoryRegion::Create(byte_size)) {} + region_(base::WritableSharedMemoryRegion::Create(byte_size)), + mapping_(region_.Map()) {} bool CefSharedProcessMessageBuilderImpl::IsValid() { - return region_.region.IsValid() && region_.mapping.IsValid(); + return region_.IsValid() && mapping_.IsValid(); } size_t CefSharedProcessMessageBuilderImpl::Size() { - return !IsValid() ? 0 : region_.mapping.size(); + return !IsValid() ? 0 : region_.GetSize(); } void* CefSharedProcessMessageBuilderImpl::Memory() { - return !IsValid() ? nullptr : region_.mapping.memory(); + return !IsValid() ? nullptr : mapping_.memory(); } CefRefPtr CefSharedProcessMessageBuilderImpl::Build() { if (!IsValid()) { return nullptr; } - return new CefProcessMessageSMRImpl(name_, std::move(region_.region)); + + // Invalidate mappping + mapping_ = base::WritableSharedMemoryMapping(); + return new CefProcessMessageSMRImpl(name_, std::move(region_)); } \ No newline at end of file diff --git a/libcef/common/process_message_smr_impl.h b/libcef/common/process_message_smr_impl.h index abc2a9f69..1f36feb86 100644 --- a/libcef/common/process_message_smr_impl.h +++ b/libcef/common/process_message_smr_impl.h @@ -9,12 +9,12 @@ #include "include/cef_process_message.h" #include "include/cef_shared_process_message_builder.h" -#include "base/memory/read_only_shared_memory_region.h" +#include "base/memory/writable_shared_memory_region.h" class CefProcessMessageSMRImpl final : public CefProcessMessage { public: CefProcessMessageSMRImpl(const CefString& name, - base::ReadOnlySharedMemoryRegion&& region); + base::WritableSharedMemoryRegion&& region); CefProcessMessageSMRImpl(const CefProcessMessageSMRImpl&) = delete; CefProcessMessageSMRImpl& operator=(const CefProcessMessageSMRImpl&) = delete; ~CefProcessMessageSMRImpl() override; @@ -26,11 +26,11 @@ class CefProcessMessageSMRImpl final : public CefProcessMessage { CefString GetName() override; CefRefPtr GetArgumentList() override { return nullptr; } CefRefPtr GetSharedMemoryRegion() override; - [[nodiscard]] base::ReadOnlySharedMemoryRegion TakeRegion(); + [[nodiscard]] base::WritableSharedMemoryRegion TakeRegion(); private: const CefString name_; - base::ReadOnlySharedMemoryRegion region_; + base::WritableSharedMemoryRegion region_; IMPLEMENT_REFCOUNTING(CefProcessMessageSMRImpl); }; @@ -50,7 +50,8 @@ class CefSharedProcessMessageBuilderImpl final private: const CefString name_; - base::MappedReadOnlyRegion region_; + base::WritableSharedMemoryRegion region_; + base::WritableSharedMemoryMapping mapping_; IMPLEMENT_REFCOUNTING(CefSharedProcessMessageBuilderImpl); }; diff --git a/libcef/renderer/frame_impl.cc b/libcef/renderer/frame_impl.cc index b5b12f7c5..cea76fa65 100644 --- a/libcef/renderer/frame_impl.cc +++ b/libcef/renderer/frame_impl.cc @@ -289,7 +289,7 @@ void CefFrameImpl::SendProcessMessage(CefProcessId target_process, SendToBrowserFrame( __FUNCTION__, base::BindOnce( - [](const CefString& name, base::ReadOnlySharedMemoryRegion region, + [](const CefString& name, base::WritableSharedMemoryRegion region, const BrowserFrameType& render_frame) { render_frame->SendSharedMemoryRegion(name, std::move(region)); }, @@ -698,7 +698,7 @@ void CefFrameImpl::SendMessage(const std::string& name, void CefFrameImpl::SendSharedMemoryRegion( const std::string& name, - base::ReadOnlySharedMemoryRegion region) { + base::WritableSharedMemoryRegion region) { if (auto app = CefAppManager::Get()->GetApplication()) { if (auto handler = app->GetRenderProcessHandler()) { CefRefPtr message( diff --git a/libcef/renderer/frame_impl.h b/libcef/renderer/frame_impl.h index c9406a882..1251e2d36 100644 --- a/libcef/renderer/frame_impl.h +++ b/libcef/renderer/frame_impl.h @@ -145,7 +145,7 @@ class CefFrameImpl void SendMessage(const std::string& name, base::Value::List arguments) override; void SendSharedMemoryRegion(const std::string& name, - base::ReadOnlySharedMemoryRegion region) override; + base::WritableSharedMemoryRegion region) override; void SendCommand(const std::string& command) override; void SendCommandWithResponse( const std::string& command, diff --git a/libcef_dll/cpptoc/shared_memory_region_cpptoc.cc b/libcef_dll/cpptoc/shared_memory_region_cpptoc.cc index fec6067f9..bebea8dd3 100644 --- a/libcef_dll/cpptoc/shared_memory_region_cpptoc.cc +++ b/libcef_dll/cpptoc/shared_memory_region_cpptoc.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=9b9187a75a85ff63f2244471af1e54f40eae5a82$ +// $hash=3bc6db85e54dc87c1e592291be01820547e0989f$ // #include "libcef_dll/cpptoc/shared_memory_region_cpptoc.h" @@ -55,7 +55,7 @@ shared_memory_region_size(struct _cef_shared_memory_region_t* self) { return _retval; } -const void* CEF_CALLBACK +void* CEF_CALLBACK shared_memory_region_memory(struct _cef_shared_memory_region_t* self) { shutdown_checker::AssertNotShutdown(); @@ -65,9 +65,9 @@ shared_memory_region_memory(struct _cef_shared_memory_region_t* self) { } // Execute - const void* _retval = CefSharedMemoryRegionCppToC::Get(self)->Memory(); + void* _retval = CefSharedMemoryRegionCppToC::Get(self)->Memory(); - // Return type: simple + // Return type: simple_byaddr return _retval; } diff --git a/libcef_dll/ctocpp/shared_memory_region_ctocpp.cc b/libcef_dll/ctocpp/shared_memory_region_ctocpp.cc index dfc3b34a7..691e272f4 100644 --- a/libcef_dll/ctocpp/shared_memory_region_ctocpp.cc +++ b/libcef_dll/ctocpp/shared_memory_region_ctocpp.cc @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=79771feab6c6d60667691c826ca9d6deaa23d068$ +// $hash=31516110398f9fe682988645d74ac8789b712181$ // #include "libcef_dll/ctocpp/shared_memory_region_ctocpp.h" @@ -51,7 +51,7 @@ NO_SANITIZE("cfi-icall") size_t CefSharedMemoryRegionCToCpp::Size() { return _retval; } -NO_SANITIZE("cfi-icall") const void* CefSharedMemoryRegionCToCpp::Memory() { +NO_SANITIZE("cfi-icall") void* CefSharedMemoryRegionCToCpp::Memory() { shutdown_checker::AssertNotShutdown(); cef_shared_memory_region_t* _struct = GetStruct(); @@ -60,9 +60,9 @@ NO_SANITIZE("cfi-icall") const void* CefSharedMemoryRegionCToCpp::Memory() { } // Execute - const void* _retval = _struct->memory(_struct); + void* _retval = _struct->memory(_struct); - // Return type: simple + // Return type: simple_byaddr return _retval; } diff --git a/libcef_dll/ctocpp/shared_memory_region_ctocpp.h b/libcef_dll/ctocpp/shared_memory_region_ctocpp.h index 45da99d96..490bfdf6e 100644 --- a/libcef_dll/ctocpp/shared_memory_region_ctocpp.h +++ b/libcef_dll/ctocpp/shared_memory_region_ctocpp.h @@ -9,7 +9,7 @@ // implementations. See the translator.README.txt file in the tools directory // for more information. // -// $hash=f5d0285d28412c40b8e04953025294c5f0779ecd$ +// $hash=a81ba6b7aca8e1f7e6e6ef41e727ddcffc06f204$ // #ifndef CEF_LIBCEF_DLL_CTOCPP_SHARED_MEMORY_REGION_CTOCPP_H_ @@ -37,7 +37,7 @@ class CefSharedMemoryRegionCToCpp // CefSharedMemoryRegion methods. bool IsValid() override; size_t Size() override; - const void* Memory() override; + void* Memory() override; }; #endif // CEF_LIBCEF_DLL_CTOCPP_SHARED_MEMORY_REGION_CTOCPP_H_ diff --git a/tests/ceftests/shared_process_message_unittest.cc b/tests/ceftests/shared_process_message_unittest.cc index ed1730aa0..7a517ae73 100644 --- a/tests/ceftests/shared_process_message_unittest.cc +++ b/tests/ceftests/shared_process_message_unittest.cc @@ -7,6 +7,7 @@ #include "tests/gtest/include/gtest/gtest.h" #include +#include namespace { @@ -29,6 +30,9 @@ CefRefPtr CreateTestBuilder() { EXPECT_NE(builder, nullptr); EXPECT_TRUE(builder->IsValid()); + static_assert( + std::is_trivially_copyable_v, + "Do not copy non-trivially-copyable object across memory spaces"); auto data = static_cast(builder->Memory()); EXPECT_NE(data, nullptr); @@ -92,3 +96,33 @@ TEST(SharedProcessMessageTest, EXPECT_EQ(read_data->buffer[i], i); } } + +TEST(SharedProcessMessageTest, WrittenValuesVisibleInOtherRegion) { + CefRefPtr read_region; + CefRefPtr write_region; + { + auto builder = CreateTestBuilder(); + auto message = builder->Build(); + read_region = message->GetSharedMemoryRegion(); + write_region = message->GetSharedMemoryRegion(); + } + + EXPECT_TRUE(write_region->IsValid()); + auto write_data = static_cast(write_region->Memory()); + write_data->flag = !kTestFlag; + write_data->value = kTestValue * 2; + auto new_double_value = kTestDoubleValue * 3; + write_data->doubleValue = new_double_value; + for (size_t i = 0; i < write_data->buffer.size(); ++i) { + write_data->buffer[i] = i + 1; + } + + EXPECT_TRUE(read_region->IsValid()); + auto read_data = static_cast(read_region->Memory()); + EXPECT_EQ(read_data->flag, !kTestFlag); + EXPECT_EQ(read_data->value, kTestValue * 2); + EXPECT_EQ(read_data->doubleValue, new_double_value); + for (size_t i = 0; i < read_data->buffer.size(); ++i) { + EXPECT_EQ(read_data->buffer[i], i + 1); + } +}