From eda086699287dcd8f6f858a5d3c0a43009dd9e59 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Fri, 8 Mar 2019 12:22:55 +0100 Subject: [PATCH] Add a C++ helper class for working with shared buffers. This is a bit more comfortable than passing the shared buffer ID manually everywhere and keeping track of size etc. --- Kernel/MemoryManager.h | 2 + Kernel/Process.cpp | 65 +++++++++++++++++++++++++---- Kernel/Process.h | 4 +- Kernel/Syscall.cpp | 6 ++- Kernel/Syscall.h | 2 + LibC/Makefile | 1 + LibC/SharedBuffer.cpp | 55 ++++++++++++++++++++++++ LibC/SharedBuffer.h | 24 +++++++++++ LibC/unistd.cpp | 14 ++++++- LibC/unistd.h | 4 +- LibGUI/GWindow.cpp | 11 ++--- SharedGraphics/GraphicsBitmap.cpp | 20 +++------ SharedGraphics/GraphicsBitmap.h | 9 ++-- WindowServer/WSClientConnection.cpp | 15 +++---- 14 files changed, 186 insertions(+), 46 deletions(-) create mode 100644 LibC/SharedBuffer.cpp create mode 100644 LibC/SharedBuffer.h diff --git a/Kernel/MemoryManager.h b/Kernel/MemoryManager.h index 25d03a004f..68ba00259f 100644 --- a/Kernel/MemoryManager.h +++ b/Kernel/MemoryManager.h @@ -203,6 +203,8 @@ public: const Bitmap& cow_map() const { return m_cow_map; } + void set_writable(bool b) { m_writable = b; } + private: RetainPtr m_page_directory; LinearAddress m_laddr; diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 4d0fa7191d..4c9aae1d11 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -2513,7 +2513,7 @@ KResult Process::wait_for_connect(Socket& socket) } struct SharedBuffer { - SharedBuffer(pid_t pid1, pid_t pid2, size_t size) + SharedBuffer(pid_t pid1, pid_t pid2, int size) : m_pid1(pid1) , m_pid2(pid2) , m_vmo(VMObject::create_anonymous(size)) @@ -2526,14 +2526,14 @@ struct SharedBuffer { if (m_pid1 == process.pid()) { ++m_pid1_retain_count; if (!m_pid1_region) { - m_pid1_region = process.allocate_region_with_vmo(LinearAddress(), size(), m_vmo.copy_ref(), 0, "SharedBuffer", true, true); + m_pid1_region = process.allocate_region_with_vmo(LinearAddress(), size(), m_vmo.copy_ref(), 0, "SharedBuffer", true, m_pid1_writable); m_pid1_region->set_shared(true); } return m_pid1_region->laddr().as_ptr(); } else if (m_pid2 == process.pid()) { ++m_pid2_retain_count; if (!m_pid2_region) { - m_pid2_region = process.allocate_region_with_vmo(LinearAddress(), size(), m_vmo.copy_ref(), 0, "SharedBuffer", true, true); + m_pid2_region = process.allocate_region_with_vmo(LinearAddress(), size(), m_vmo.copy_ref(), 0, "SharedBuffer", true, m_pid2_writable); m_pid2_region->set_shared(true); } return m_pid2_region->laddr().as_ptr(); @@ -2584,6 +2584,20 @@ struct SharedBuffer { size_t size() const { return m_vmo->size(); } void destroy_if_unused(); + void seal() + { + m_pid1_writable = false; + m_pid2_writable = false; + if (m_pid1_region) { + m_pid1_region->set_writable(false); + MM.remap_region(*m_pid1_region->page_directory(), *m_pid1_region); + } + if (m_pid2_region) { + m_pid2_region->set_writable(false); + MM.remap_region(*m_pid2_region->page_directory(), *m_pid2_region); + } + } + int m_shared_buffer_id { -1 }; pid_t m_pid1; pid_t m_pid2; @@ -2591,6 +2605,8 @@ struct SharedBuffer { unsigned m_pid2_retain_count { 0 }; Region* m_pid1_region { nullptr }; Region* m_pid2_region { nullptr }; + bool m_pid1_writable { false }; + bool m_pid2_writable { false }; Retained m_vmo; }; @@ -2608,7 +2624,7 @@ void SharedBuffer::destroy_if_unused() if (!m_pid1_retain_count && !m_pid2_retain_count) { LOCKER(shared_buffers().lock()); #ifdef SHARED_BUFFER_DEBUG - dbgprintf("Destroying unused SharedBuffer{%p} id: %d (pid1: %d, pid2: %d)\n", this, m_shared_buffer_id, m_pid1, m_pid2); + kprintf("Destroying unused SharedBuffer{%p} id: %d (pid1: %d, pid2: %d)\n", this, m_shared_buffer_id, m_pid1, m_pid2); #endif size_t count_before = shared_buffers().resource().size(); shared_buffers().resource().remove(m_shared_buffer_id); @@ -2626,9 +2642,9 @@ void Process::disown_all_shared_buffers() shared_buffer->disown(m_pid); } -int Process::sys$create_shared_buffer(pid_t peer_pid, size_t size, void** buffer) +int Process::sys$create_shared_buffer(pid_t peer_pid, int size, void** buffer) { - if (!size) + if (!size || size < 0) return -EINVAL; size = PAGE_ROUND_UP(size); if (!peer_pid || peer_pid < 0 || peer_pid == m_pid) @@ -2650,7 +2666,7 @@ int Process::sys$create_shared_buffer(pid_t peer_pid, size_t size, void** buffer shared_buffer->m_pid1_region->set_shared(true); *buffer = shared_buffer->m_pid1_region->laddr().as_ptr(); #ifdef SHARED_BUFFER_DEBUG - dbgprintf("%s(%u): Created shared buffer %d (%u bytes, vmo is %u) for sharing with %d\n", name().characters(), pid(),shared_buffer_id, size, shared_buffer->size(), peer_pid); + kprintf("%s(%u): Created shared buffer %d (%u bytes, vmo is %u) for sharing with %d\n", name().characters(), pid(),shared_buffer_id, size, shared_buffer->size(), peer_pid); #endif shared_buffers().resource().set(shared_buffer_id, move(shared_buffer)); return shared_buffer_id; @@ -2664,7 +2680,7 @@ int Process::sys$release_shared_buffer(int shared_buffer_id) return -EINVAL; auto& shared_buffer = *(*it).value; #ifdef SHARED_BUFFER_DEBUG - dbgprintf("%s(%u): Releasing shared buffer %d, buffer count: %u\n", name().characters(), pid(), shared_buffer_id, shared_buffers().resource().size()); + kprintf("%s(%u): Releasing shared buffer %d, buffer count: %u\n", name().characters(), pid(), shared_buffer_id, shared_buffers().resource().size()); #endif shared_buffer.release(*this); return 0; @@ -2680,11 +2696,42 @@ void* Process::sys$get_shared_buffer(int shared_buffer_id) if (shared_buffer.pid1() != m_pid && shared_buffer.pid2() != m_pid) return (void*)-EINVAL; #ifdef SHARED_BUFFER_DEBUG - dbgprintf("%s(%u): Retaining shared buffer %d, buffer count: %u\n", name().characters(), pid(), shared_buffer_id, shared_buffers().resource().size()); + kprintf("%s(%u): Retaining shared buffer %d, buffer count: %u\n", name().characters(), pid(), shared_buffer_id, shared_buffers().resource().size()); #endif return shared_buffer.retain(*this); } +int Process::sys$seal_shared_buffer(int shared_buffer_id) +{ + LOCKER(shared_buffers().lock()); + auto it = shared_buffers().resource().find(shared_buffer_id); + if (it == shared_buffers().resource().end()) + return -EINVAL; + auto& shared_buffer = *(*it).value; + if (shared_buffer.pid1() != m_pid && shared_buffer.pid2() != m_pid) + return -EINVAL; +#ifdef SHARED_BUFFER_DEBUG + kprintf("%s(%u): Sealing shared buffer %d\n", name().characters(), pid(), shared_buffer_id); +#endif + shared_buffer.seal(); + return 0; +} + +int Process::sys$get_shared_buffer_size(int shared_buffer_id) +{ + LOCKER(shared_buffers().lock()); + auto it = shared_buffers().resource().find(shared_buffer_id); + if (it == shared_buffers().resource().end()) + return -EINVAL; + auto& shared_buffer = *(*it).value; + if (shared_buffer.pid1() != m_pid && shared_buffer.pid2() != m_pid) + return -EINVAL; +#ifdef SHARED_BUFFER_DEBUG + kprintf("%s(%u): Get shared buffer %d size: %u\n", name().characters(), pid(), shared_buffer_id, shared_buffers().resource().size()); +#endif + return shared_buffer.size(); +} + const char* to_string(Process::State state) { switch (state) { diff --git a/Kernel/Process.h b/Kernel/Process.h index 1130ded9c0..6c9a17ae20 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -232,9 +232,11 @@ public: int sys$connect(int sockfd, const sockaddr*, socklen_t); int sys$restore_signal_mask(dword mask); - int sys$create_shared_buffer(pid_t peer_pid, size_t, void** buffer); + int sys$create_shared_buffer(pid_t peer_pid, int, void** buffer); void* sys$get_shared_buffer(int shared_buffer_id); int sys$release_shared_buffer(int shared_buffer_id); + int sys$seal_shared_buffer(int shared_buffer_id); + int sys$get_shared_buffer_size(int shared_buffer_id); KResult wait_for_connect(Socket&); diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp index 945be0e679..2f0cb277c9 100644 --- a/Kernel/Syscall.cpp +++ b/Kernel/Syscall.cpp @@ -223,8 +223,12 @@ static dword handle(RegisterDump& regs, dword function, dword arg1, dword arg2, return current->sys$chown((const char*)arg1, (uid_t)arg2, (gid_t)arg3); case Syscall::SC_restore_signal_mask: return current->sys$restore_signal_mask((dword)arg1); + case Syscall::SC_seal_shared_buffer: + return current->sys$seal_shared_buffer((int)arg1); + case Syscall::SC_get_shared_buffer_size: + return current->sys$get_shared_buffer_size((int)arg1); default: - kprintf("<%u> int0x80: Unknown function %u requested {%x, %x, %x}\n", current->pid(), function, arg1, arg2, arg3); + kprintf("<%u> int0x82: Unknown function %u requested {%x, %x, %x}\n", current->pid(), function, arg1, arg2, arg3); break; } return 0; diff --git a/Kernel/Syscall.h b/Kernel/Syscall.h index f4e3255957..4a767b0f5f 100644 --- a/Kernel/Syscall.h +++ b/Kernel/Syscall.h @@ -86,6 +86,8 @@ __ENUMERATE_SYSCALL(fchmod) \ __ENUMERATE_SYSCALL(symlink) \ __ENUMERATE_SYSCALL(restore_signal_mask) \ + __ENUMERATE_SYSCALL(get_shared_buffer_size) \ + __ENUMERATE_SYSCALL(seal_shared_buffer) \ namespace Syscall { diff --git a/LibC/Makefile b/LibC/Makefile index 04dbfd7b2b..c11873f17d 100644 --- a/LibC/Makefile +++ b/LibC/Makefile @@ -7,6 +7,7 @@ AK_OBJS = \ ../AK/kmalloc.o LIBC_OBJS = \ + SharedBuffer.o \ stdio.o \ unistd.o \ string.o \ diff --git a/LibC/SharedBuffer.cpp b/LibC/SharedBuffer.cpp new file mode 100644 index 0000000000..dad0841499 --- /dev/null +++ b/LibC/SharedBuffer.cpp @@ -0,0 +1,55 @@ +#include +#include +#include + +RetainPtr SharedBuffer::create(pid_t peer, int size) +{ + void* data; + int shared_buffer_id = create_shared_buffer(peer, size, &data); + if (shared_buffer_id < 0) { + perror("create_shared_buffer"); + return nullptr; + } + return adopt(*new SharedBuffer(shared_buffer_id, size, data)); +} + +RetainPtr SharedBuffer::create_from_shared_buffer_id(int shared_buffer_id) +{ + void* data = get_shared_buffer(shared_buffer_id); + if (data == (void*)-1) { + perror("get_shared_buffer"); + return nullptr; + } + int size = get_shared_buffer_size(shared_buffer_id); + if (size < 0) { + perror("get_shared_buffer_size"); + return nullptr; + } + return adopt(*new SharedBuffer(shared_buffer_id, size, data)); +} + +SharedBuffer::SharedBuffer(int shared_buffer_id, int size, void* data) + : m_shared_buffer_id(shared_buffer_id) + , m_size(size) + , m_data(data) +{ +} + +SharedBuffer::~SharedBuffer() +{ + if (m_shared_buffer_id >= 0) { + int rc = release_shared_buffer(m_shared_buffer_id); + if (rc < 0) { + perror("release_shared_buffer"); + } + } +} + +void SharedBuffer::seal() +{ + int rc = seal_shared_buffer(m_shared_buffer_id); + if (rc < 0) { + perror("seal_shared_buffer"); + exit(1); + } +} diff --git a/LibC/SharedBuffer.h b/LibC/SharedBuffer.h new file mode 100644 index 0000000000..33de7c7a6e --- /dev/null +++ b/LibC/SharedBuffer.h @@ -0,0 +1,24 @@ +#pragma once + +#include +#include + +class SharedBuffer : public Retainable { +public: + static RetainPtr create(pid_t peer, int); + static RetainPtr create_from_shared_buffer_id(int); + ~SharedBuffer(); + + int shared_buffer_id() const { return m_shared_buffer_id; } + void seal(); + int size() const { return m_size; } + void* data() { return m_data; } + const void* data() const { return m_data; } + +private: + SharedBuffer(int shared_buffer_id, int size, void*); + + int m_shared_buffer_id { -1 }; + int m_size { 0 }; + void* m_data; +}; diff --git a/LibC/unistd.cpp b/LibC/unistd.cpp index 6a749e3aa1..e5e2e90fbe 100644 --- a/LibC/unistd.cpp +++ b/LibC/unistd.cpp @@ -361,7 +361,7 @@ int read_tsc(unsigned* lsw, unsigned* msw) __RETURN_WITH_ERRNO(rc, rc, -1); } -int create_shared_buffer(pid_t peer_pid, size_t size, void** buffer) +int create_shared_buffer(pid_t peer_pid, int size, void** buffer) { int rc = syscall(SC_create_shared_buffer, peer_pid, size, buffer); __RETURN_WITH_ERRNO(rc, rc, -1); @@ -383,6 +383,18 @@ int release_shared_buffer(int shared_buffer_id) __RETURN_WITH_ERRNO(rc, rc, -1); } +int get_shared_buffer_size(int shared_buffer_id) +{ + int rc = syscall(SC_get_shared_buffer_size, shared_buffer_id); + __RETURN_WITH_ERRNO(rc, rc, -1); +} + +int seal_shared_buffer(int shared_buffer_id) +{ + int rc = syscall(SC_seal_shared_buffer, shared_buffer_id); + __RETURN_WITH_ERRNO(rc, rc, -1); +} + char* getlogin() { assert(false); diff --git a/LibC/unistd.h b/LibC/unistd.h index 232f11bafc..a5ce7936fe 100644 --- a/LibC/unistd.h +++ b/LibC/unistd.h @@ -14,9 +14,11 @@ __BEGIN_DECLS extern char** environ; -int create_shared_buffer(pid_t peer_pid, size_t, void** buffer); +int create_shared_buffer(pid_t peer_pid, int, void** buffer); void* get_shared_buffer(int shared_buffer_id); int release_shared_buffer(int shared_buffer_id); +int seal_shared_buffer(int shared_buffer_id); +int get_shared_buffer_size(int shared_buffer_id); int read_tsc(unsigned* lsw, unsigned* msw); inline int getpagesize() { return 4096; } pid_t fork(); diff --git a/LibGUI/GWindow.cpp b/LibGUI/GWindow.cpp index 84458fc503..39cd74d179 100644 --- a/LibGUI/GWindow.cpp +++ b/LibGUI/GWindow.cpp @@ -172,15 +172,12 @@ void GWindow::event(GEvent& event) ASSERT(!paint_event.window_size().is_empty()); Size new_backing_store_size = paint_event.window_size(); size_t size_in_bytes = new_backing_store_size.area() * sizeof(RGBA32); - void* buffer; - int shared_buffer_id = create_shared_buffer(GEventLoop::main().server_pid(), size_in_bytes, (void**)&buffer); - ASSERT(shared_buffer_id >= 0); - ASSERT(buffer); - ASSERT(buffer != (void*)-1); + auto shared_buffer = SharedBuffer::create(GEventLoop::main().server_pid(), size_in_bytes); + ASSERT(shared_buffer); m_backing = GraphicsBitmap::create_with_shared_buffer( m_has_alpha_channel ? GraphicsBitmap::Format::RGBA32 : GraphicsBitmap::Format::RGB32, - shared_buffer_id, - new_backing_store_size, (RGBA32*)buffer); + *shared_buffer, + new_backing_store_size); } if (rect.is_empty() || created_new_backing_store) rect = m_main_widget->rect(); diff --git a/SharedGraphics/GraphicsBitmap.cpp b/SharedGraphics/GraphicsBitmap.cpp index 035f17e9a4..1df228062d 100644 --- a/SharedGraphics/GraphicsBitmap.cpp +++ b/SharedGraphics/GraphicsBitmap.cpp @@ -58,23 +58,17 @@ GraphicsBitmap::GraphicsBitmap(Format format, const Size& size, RGBA32* data) { } -RetainPtr GraphicsBitmap::create_with_shared_buffer(Format format, int shared_buffer_id, const Size& size, RGBA32* data) +RetainPtr GraphicsBitmap::create_with_shared_buffer(Format format, Retained&& shared_buffer, const Size& size) { - if (!data) { - void* shared_buffer = get_shared_buffer(shared_buffer_id); - if (!shared_buffer || shared_buffer == (void*)-1) - return nullptr; - data = (RGBA32*)shared_buffer; - } - return adopt(*new GraphicsBitmap(format, shared_buffer_id, size, data)); + return adopt(*new GraphicsBitmap(format, move(shared_buffer), size)); } -GraphicsBitmap::GraphicsBitmap(Format format, int shared_buffer_id, const Size& size, RGBA32* data) +GraphicsBitmap::GraphicsBitmap(Format format, Retained&& shared_buffer, const Size& size) : m_size(size) - , m_data(data) + , m_data((RGBA32*)shared_buffer->data()) , m_pitch(size.width() * sizeof(RGBA32)) , m_format(format) - , m_shared_buffer_id(shared_buffer_id) + , m_shared_buffer(move(shared_buffer)) { } @@ -84,10 +78,6 @@ GraphicsBitmap::~GraphicsBitmap() int rc = munmap(m_data, m_size.area() * 4); ASSERT(rc == 0); } - if (m_shared_buffer_id != -1) { - int rc = release_shared_buffer(m_shared_buffer_id); - ASSERT(rc == 0); - } m_data = nullptr; } diff --git a/SharedGraphics/GraphicsBitmap.h b/SharedGraphics/GraphicsBitmap.h index 792e0d0076..156eb0f2d0 100644 --- a/SharedGraphics/GraphicsBitmap.h +++ b/SharedGraphics/GraphicsBitmap.h @@ -6,6 +6,7 @@ #include #include #include +#include class GraphicsBitmap : public Retainable { public: @@ -14,7 +15,7 @@ public: static Retained create(Format, const Size&); static Retained create_wrapper(Format, const Size&, RGBA32*); static RetainPtr load_from_file(Format, const String& path, const Size&); - static RetainPtr create_with_shared_buffer(Format, int shared_buffer_id, const Size&, RGBA32* buffer = nullptr); + static RetainPtr create_with_shared_buffer(Format, Retained&&, const Size&); ~GraphicsBitmap(); RGBA32* scanline(int y); @@ -25,21 +26,21 @@ public: int width() const { return m_size.width(); } int height() const { return m_size.height(); } size_t pitch() const { return m_pitch; } - int shared_buffer_id() const { return m_shared_buffer_id; } + int shared_buffer_id() const { return m_shared_buffer ? m_shared_buffer->shared_buffer_id() : -1; } bool has_alpha_channel() const { return m_format == Format::RGBA32; } private: GraphicsBitmap(Format, const Size&); GraphicsBitmap(Format, const Size&, RGBA32*); - GraphicsBitmap(Format, int shared_buffer_id, const Size&, RGBA32*); + GraphicsBitmap(Format, Retained&&, const Size&); Size m_size; RGBA32* m_data { nullptr }; size_t m_pitch { 0 }; Format m_format { Format::Invalid }; bool m_mmaped { false }; - int m_shared_buffer_id { -1 }; + RetainPtr m_shared_buffer; }; inline RGBA32* GraphicsBitmap::scanline(int y) diff --git a/WindowServer/WSClientConnection.cpp b/WindowServer/WSClientConnection.cpp index 71c4c8a076..082326df66 100644 --- a/WindowServer/WSClientConnection.cpp +++ b/WindowServer/WSClientConnection.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -86,12 +87,9 @@ void WSClientConnection::post_message(const WSAPI_ServerMessage& message) RetainPtr WSClientConnection::create_shared_bitmap(GraphicsBitmap::Format format, const Size& size) { - RGBA32* buffer; - int shared_buffer_id = create_shared_buffer(m_pid, size.area() * sizeof(RGBA32), (void**)&buffer); - ASSERT(shared_buffer_id >= 0); - ASSERT(buffer); - ASSERT(buffer != (void*)-1); - return GraphicsBitmap::create_with_shared_buffer(format, shared_buffer_id, size, buffer); + auto shared_buffer = SharedBuffer::create(m_pid, size.area() * sizeof(RGBA32)); + ASSERT(shared_buffer); + return GraphicsBitmap::create_with_shared_buffer(format, *shared_buffer, size); } void WSClientConnection::on_message(WSMessage& message) @@ -407,9 +405,12 @@ void WSClientConnection::handle_request(WSAPISetWindowBackingStoreRequest& reque return; } auto& window = *(*it).value; + auto shared_buffer = SharedBuffer::create_from_shared_buffer_id(request.shared_buffer_id()); + if (!shared_buffer) + return; auto backing_store = GraphicsBitmap::create_with_shared_buffer( request.has_alpha_channel() ? GraphicsBitmap::Format::RGBA32 : GraphicsBitmap::Format::RGB32, - request.shared_buffer_id(), + *shared_buffer, request.size()); if (!backing_store) return;