From 6d4ba21832abcdcfecc6a4b75cfb017b729322f0 Mon Sep 17 00:00:00 2001 From: Andrew Kaster Date: Tue, 16 Apr 2024 13:46:30 -0600 Subject: [PATCH] LibIPC+Userland: Make IPC::File always own its file descriptor Add factory functions to distinguish between when the owner of the File wants to transfer ownership to the new IPC object (adopt) or to send a copy of the same fd to the IPC peer (clone). This behavior is more intuitive than the previous behavior. Previously, an IPC::File would default to a shallow clone of the file descriptor, only *actually* calling dup(2) for the fd when encoding or it into an IPC MessageBuffer. Now the dup(2) for the fd is explicit in the clone_fd factory function. --- .../DevTools/HackStudio/LanguageClient.cpp | 2 +- Userland/Libraries/LibGUI/Window.cpp | 2 +- Userland/Libraries/LibGfx/ShareableBitmap.cpp | 2 +- Userland/Libraries/LibIPC/Decoder.cpp | 2 +- Userland/Libraries/LibIPC/Encoder.cpp | 7 +--- Userland/Libraries/LibIPC/Encoder.h | 4 +- Userland/Libraries/LibIPC/File.h | 37 ++++++++----------- .../Libraries/LibWeb/HTML/MessagePort.cpp | 4 +- .../Libraries/LibWeb/HTML/SelectedFile.cpp | 2 +- Userland/Libraries/LibWeb/Page/Page.h | 2 +- .../LibWeb/Worker/WebWorkerClient.cpp | 4 +- .../LibWebView/OutOfProcessWebView.cpp | 2 +- .../LibWebView/ViewImplementation.cpp | 2 +- .../Libraries/LibWebView/WebContentClient.cpp | 2 +- .../ConnectionFromClient.cpp | 4 +- .../RequestServer/ConnectionFromClient.cpp | 14 ++++--- .../WebWorker/ConnectionFromClient.cpp | 2 +- 17 files changed, 44 insertions(+), 50 deletions(-) diff --git a/Userland/DevTools/HackStudio/LanguageClient.cpp b/Userland/DevTools/HackStudio/LanguageClient.cpp index 54c4f9593ad..58af26c956d 100644 --- a/Userland/DevTools/HackStudio/LanguageClient.cpp +++ b/Userland/DevTools/HackStudio/LanguageClient.cpp @@ -64,7 +64,7 @@ void LanguageClient::open_file(ByteString const& path, int fd) { if (!m_connection_wrapper.connection()) return; - m_connection_wrapper.connection()->async_file_opened(path, fd); + m_connection_wrapper.connection()->async_file_opened(path, MUST(IPC::File::clone_fd(fd))); } void LanguageClient::set_file_content(ByteString const& path, ByteString const& content) diff --git a/Userland/Libraries/LibGUI/Window.cpp b/Userland/Libraries/LibGUI/Window.cpp index f274f4774de..d79b239c265 100644 --- a/Userland/Libraries/LibGUI/Window.cpp +++ b/Userland/Libraries/LibGUI/Window.cpp @@ -983,7 +983,7 @@ void Window::set_current_backing_store(WindowBackingStore& backing_store, bool f m_window_id, 32, bitmap.pitch(), - bitmap.anonymous_buffer().fd(), + MUST(IPC::File::clone_fd(bitmap.anonymous_buffer().fd())), backing_store.serial(), bitmap.has_alpha_channel(), bitmap.size(), diff --git a/Userland/Libraries/LibGfx/ShareableBitmap.cpp b/Userland/Libraries/LibGfx/ShareableBitmap.cpp index 26f87a2195b..c8e4df017f4 100644 --- a/Userland/Libraries/LibGfx/ShareableBitmap.cpp +++ b/Userland/Libraries/LibGfx/ShareableBitmap.cpp @@ -30,7 +30,7 @@ ErrorOr encode(Encoder& encoder, Gfx::ShareableBitmap const& shareable_bit return {}; auto& bitmap = *shareable_bitmap.bitmap(); - TRY(encoder.encode(IPC::File(bitmap.anonymous_buffer().fd()))); + TRY(encoder.encode(TRY(IPC::File::clone_fd(bitmap.anonymous_buffer().fd())))); TRY(encoder.encode(bitmap.size())); TRY(encoder.encode(static_cast(bitmap.scale()))); TRY(encoder.encode(static_cast(bitmap.format()))); diff --git a/Userland/Libraries/LibIPC/Decoder.cpp b/Userland/Libraries/LibIPC/Decoder.cpp index 3e2ac066776..1abd370ce5c 100644 --- a/Userland/Libraries/LibIPC/Decoder.cpp +++ b/Userland/Libraries/LibIPC/Decoder.cpp @@ -91,7 +91,7 @@ template<> ErrorOr decode(Decoder& decoder) { int fd = TRY(decoder.socket().receive_fd(O_CLOEXEC)); - return File { fd, File::ConstructWithReceivedFileDescriptor }; + return File::adopt_fd(fd); } template<> diff --git a/Userland/Libraries/LibIPC/Encoder.cpp b/Userland/Libraries/LibIPC/Encoder.cpp index 0469eba2e14..51ac2efbeb2 100644 --- a/Userland/Libraries/LibIPC/Encoder.cpp +++ b/Userland/Libraries/LibIPC/Encoder.cpp @@ -105,10 +105,7 @@ ErrorOr encode(Encoder& encoder, URL::URL const& value) template<> ErrorOr encode(Encoder& encoder, File const& file) { - int fd = file.fd(); - - if (fd != -1) - fd = TRY(Core::System::dup(fd)); + int fd = file.take_fd(); TRY(encoder.append_file_descriptor(fd)); return {}; @@ -127,7 +124,7 @@ ErrorOr encode(Encoder& encoder, Core::AnonymousBuffer const& buffer) if (buffer.is_valid()) { TRY(encoder.encode_size(buffer.size())); - TRY(encoder.encode(IPC::File { buffer.fd() })); + TRY(encoder.encode(TRY(IPC::File::clone_fd(buffer.fd())))); } return {}; diff --git a/Userland/Libraries/LibIPC/Encoder.h b/Userland/Libraries/LibIPC/Encoder.h index d3364f9cfbc..6813c8af3fc 100644 --- a/Userland/Libraries/LibIPC/Encoder.h +++ b/Userland/Libraries/LibIPC/Encoder.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -148,7 +149,8 @@ ErrorOr encode(Encoder& encoder, T const& hashmap) template ErrorOr encode(Encoder& encoder, T const& queue) { - return encoder.encode(IPC::File { queue.fd() }); + TRY(encoder.encode(TRY(IPC::File::clone_fd(queue.fd())))); + return {}; } template diff --git a/Userland/Libraries/LibIPC/File.h b/Userland/Libraries/LibIPC/File.h index 15f70a69d8e..ab565e7a400 100644 --- a/Userland/Libraries/LibIPC/File.h +++ b/Userland/Libraries/LibIPC/File.h @@ -10,7 +10,7 @@ #include #include #include -#include +#include namespace IPC { @@ -18,36 +18,26 @@ class File { AK_MAKE_NONCOPYABLE(File); public: - // Must have a default constructor, because LibIPC - // default-constructs arguments prior to decoding them. File() = default; - // Intentionally not `explicit`. - File(int fd) - : m_fd(fd) + static File adopt_file(NonnullOwnPtr file) { + return File(file->leak_fd(Badge {})); } - // Tagged constructor for fd's that should be closed on destruction unless take_fd() is called. - // Note that the tags are the same, this is intentional to allow expressive invocation. - enum Tag { - ConstructWithReceivedFileDescriptor = 1, - CloseAfterSending = 1, - }; - File(int fd, Tag) - : m_fd(fd) - , m_close_on_destruction(true) + static File adopt_fd(int fd) { + return File(fd); } - explicit File(Core::File& file) - : File(file.leak_fd(Badge {}), CloseAfterSending) + static ErrorOr clone_fd(int fd) { + int new_fd = TRY(Core::System::dup(fd)); + return File(new_fd); } File(File&& other) : m_fd(exchange(other.m_fd, -1)) - , m_close_on_destruction(exchange(other.m_close_on_destruction, false)) { } @@ -55,15 +45,14 @@ public: { if (this != &other) { m_fd = exchange(other.m_fd, -1); - m_close_on_destruction = exchange(other.m_close_on_destruction, false); } return *this; } ~File() { - if (m_close_on_destruction && m_fd != -1) - close(m_fd); + if (m_fd != -1) + (void)Core::System::close(m_fd); } int fd() const { return m_fd; } @@ -75,8 +64,12 @@ public: } private: + explicit File(int fd) + : m_fd(fd) + { + } + mutable int m_fd { -1 }; - bool m_close_on_destruction { false }; }; } diff --git a/Userland/Libraries/LibWeb/HTML/MessagePort.cpp b/Userland/Libraries/LibWeb/HTML/MessagePort.cpp index 9c95839ce0b..6cf22e73735 100644 --- a/Userland/Libraries/LibWeb/HTML/MessagePort.cpp +++ b/Userland/Libraries/LibWeb/HTML/MessagePort.cpp @@ -80,12 +80,12 @@ WebIDL::ExceptionOr MessagePort::transfer_steps(HTML::TransferDataHolder& // 2. Set dataHolder.[[RemotePort]] to remotePort. auto fd = MUST(m_socket->release_fd()); m_socket = nullptr; - data_holder.fds.append(IPC::File(fd, IPC::File::CloseAfterSending)); + data_holder.fds.append(IPC::File::adopt_fd(fd)); data_holder.data.append(IPC_FILE_TAG); auto fd_passing_socket = MUST(m_fd_passing_socket->release_fd()); m_fd_passing_socket = nullptr; - data_holder.fds.append(IPC::File(fd_passing_socket, IPC::File::CloseAfterSending)); + data_holder.fds.append(IPC::File::adopt_fd(fd_passing_socket)); data_holder.data.append(IPC_FILE_TAG); } diff --git a/Userland/Libraries/LibWeb/HTML/SelectedFile.cpp b/Userland/Libraries/LibWeb/HTML/SelectedFile.cpp index be31a4e1b32..5b059398a90 100644 --- a/Userland/Libraries/LibWeb/HTML/SelectedFile.cpp +++ b/Userland/Libraries/LibWeb/HTML/SelectedFile.cpp @@ -20,7 +20,7 @@ ErrorOr SelectedFile::from_file_path(ByteString const& file_path) auto name = LexicalPath::basename(file_path); auto file = TRY(Core::File::open(file_path, Core::File::OpenMode::Read)); - return SelectedFile { move(name), IPC::File { *file } }; + return SelectedFile { move(name), IPC::File::adopt_file(move(file)) }; } SelectedFile::SelectedFile(ByteString name, ByteBuffer contents) diff --git a/Userland/Libraries/LibWeb/Page/Page.h b/Userland/Libraries/LibWeb/Page/Page.h index 7f60ab4ad24..8df6cdd40a8 100644 --- a/Userland/Libraries/LibWeb/Page/Page.h +++ b/Userland/Libraries/LibWeb/Page/Page.h @@ -315,7 +315,7 @@ public: virtual void page_did_change_audio_play_state(HTML::AudioPlayState) { } - virtual WebView::SocketPair request_worker_agent() { return { -1, -1 }; } + virtual WebView::SocketPair request_worker_agent() { return { IPC::File {}, IPC::File {} }; } virtual void inspector_did_load() { } virtual void inspector_did_select_dom_node([[maybe_unused]] i32 node_id, [[maybe_unused]] Optional const& pseudo_element) { } diff --git a/Userland/Libraries/LibWeb/Worker/WebWorkerClient.cpp b/Userland/Libraries/LibWeb/Worker/WebWorkerClient.cpp index 418f97fd57e..943b23c140b 100644 --- a/Userland/Libraries/LibWeb/Worker/WebWorkerClient.cpp +++ b/Userland/Libraries/LibWeb/Worker/WebWorkerClient.cpp @@ -22,8 +22,8 @@ WebWorkerClient::WebWorkerClient(NonnullOwnPtr socket) WebView::SocketPair WebWorkerClient::dup_sockets() { WebView::SocketPair pair; - pair.socket = IPC::File(MUST(Core::System::dup(socket().fd().value())), IPC::File::CloseAfterSending); - pair.fd_passing_socket = IPC::File(MUST(Core::System::dup(fd_passing_socket().fd().value())), IPC::File::CloseAfterSending); + pair.socket = MUST(IPC::File::clone_fd(socket().fd().value())); + pair.fd_passing_socket = MUST(IPC::File::clone_fd(fd_passing_socket().fd().value())); return pair; } diff --git a/Userland/Libraries/LibWebView/OutOfProcessWebView.cpp b/Userland/Libraries/LibWebView/OutOfProcessWebView.cpp index 981666f75f7..a2c1ab518c2 100644 --- a/Userland/Libraries/LibWebView/OutOfProcessWebView.cpp +++ b/Userland/Libraries/LibWebView/OutOfProcessWebView.cpp @@ -44,7 +44,7 @@ OutOfProcessWebView::OutOfProcessWebView() if (file.is_error()) client().async_handle_file_return(m_client_state.page_index, file.error().code(), {}, request_id); else - client().async_handle_file_return(m_client_state.page_index, 0, IPC::File(file.value().stream()), request_id); + client().async_handle_file_return(m_client_state.page_index, 0, IPC::File::adopt_file(file.release_value().release_stream()), request_id); }; on_scroll_by_delta = [this](auto x_delta, auto y_delta) { diff --git a/Userland/Libraries/LibWebView/ViewImplementation.cpp b/Userland/Libraries/LibWebView/ViewImplementation.cpp index c70a91bc106..bba1b31e6ae 100644 --- a/Userland/Libraries/LibWebView/ViewImplementation.cpp +++ b/Userland/Libraries/LibWebView/ViewImplementation.cpp @@ -32,7 +32,7 @@ ViewImplementation::ViewImplementation() if (file.is_error()) client().async_handle_file_return(page_id(), file.error().code(), {}, request_id); else - client().async_handle_file_return(page_id(), 0, IPC::File(*file.value()), request_id); + client().async_handle_file_return(page_id(), 0, IPC::File::adopt_file(file.release_value()), request_id); }; } diff --git a/Userland/Libraries/LibWebView/WebContentClient.cpp b/Userland/Libraries/LibWebView/WebContentClient.cpp index 3601d931dc0..609eb9d2c2c 100644 --- a/Userland/Libraries/LibWebView/WebContentClient.cpp +++ b/Userland/Libraries/LibWebView/WebContentClient.cpp @@ -665,7 +665,7 @@ Messages::WebContentClient::RequestWorkerAgentResponse WebContentClient::request return view->on_request_worker_agent(); } - return WebView::SocketPair { -1, -1 }; + return WebView::SocketPair { IPC::File {}, IPC::File {} }; } Optional WebContentClient::view_for_page_id(u64 page_id, SourceLocation location) diff --git a/Userland/Services/FileSystemAccessServer/ConnectionFromClient.cpp b/Userland/Services/FileSystemAccessServer/ConnectionFromClient.cpp index a22d413007f..f8852f1beb4 100644 --- a/Userland/Services/FileSystemAccessServer/ConnectionFromClient.cpp +++ b/Userland/Services/FileSystemAccessServer/ConnectionFromClient.cpp @@ -81,7 +81,7 @@ void ConnectionFromClient::request_file_handler(i32 request_id, i32 window_serve dbgln("FileSystemAccessServer: Couldn't open {}, error {}", path, file.error()); async_handle_prompt_end(request_id, file.error().code(), Optional {}, path); } else { - async_handle_prompt_end(request_id, 0, IPC::File(*file.release_value()), path); + async_handle_prompt_end(request_id, 0, IPC::File::adopt_file(file.release_value()), path); } } else { async_handle_prompt_end(request_id, EPERM, Optional {}, path); @@ -138,7 +138,7 @@ void ConnectionFromClient::prompt_helper(i32 request_id, Optional co m_approved_files.set(user_picked_file.value(), new_permissions); - async_handle_prompt_end(request_id, 0, IPC::File(*file.release_value()), user_picked_file); + async_handle_prompt_end(request_id, 0, IPC::File::adopt_file(file.release_value()), user_picked_file); } } else { async_handle_prompt_end(request_id, ECANCELED, Optional {}, Optional {}); diff --git a/Userland/Services/RequestServer/ConnectionFromClient.cpp b/Userland/Services/RequestServer/ConnectionFromClient.cpp index 4ee6e093fc9..ca01cd995bb 100644 --- a/Userland/Services/RequestServer/ConnectionFromClient.cpp +++ b/Userland/Services/RequestServer/ConnectionFromClient.cpp @@ -42,10 +42,12 @@ void ConnectionFromClient::die() Messages::RequestServer::ConnectNewClientResponse ConnectionFromClient::connect_new_client() { + Messages::RequestServer::ConnectNewClientResponse error_response = { IPC::File {}, IPC::File {} }; + int socket_fds[2] {}; if (auto err = Core::System::socketpair(AF_LOCAL, SOCK_STREAM, 0, socket_fds); err.is_error()) { dbgln("Failed to create client socketpair: {}", err.error()); - return { -1, -1 }; + return error_response; } auto client_socket_or_error = Core::LocalSocket::adopt_fd(socket_fds[0]); @@ -53,7 +55,7 @@ Messages::RequestServer::ConnectNewClientResponse ConnectionFromClient::connect_ close(socket_fds[0]); close(socket_fds[1]); dbgln("Failed to adopt client socket: {}", client_socket_or_error.error()); - return { -1, -1 }; + return error_response; } auto client_socket = client_socket_or_error.release_value(); // Note: A ref is stored in the static s_connections map @@ -63,7 +65,7 @@ Messages::RequestServer::ConnectNewClientResponse ConnectionFromClient::connect_ if (auto err = Core::System::socketpair(AF_LOCAL, SOCK_STREAM, 0, fd_passing_socket_fds); err.is_error()) { close(socket_fds[1]); dbgln("Failed to create fd-passing socketpair: {}", err.error()); - return { -1, -1 }; + return error_response; } auto fd_passing_socket_or_error = Core::LocalSocket::adopt_fd(fd_passing_socket_fds[0]); @@ -73,12 +75,12 @@ Messages::RequestServer::ConnectNewClientResponse ConnectionFromClient::connect_ close(fd_passing_socket_fds[0]); close(fd_passing_socket_fds[1]); dbgln("Failed to adopt fd-passing socket: {}", fd_passing_socket_or_error.error()); - return { -1, -1 }; + return error_response; } auto fd_passing_socket = fd_passing_socket_or_error.release_value(); client->set_fd_passing_socket(move(fd_passing_socket)); - return { IPC::File(socket_fds[1], IPC::File::CloseAfterSending), IPC::File(fd_passing_socket_fds[1], IPC::File::CloseAfterSending) }; + return { IPC::File::adopt_fd(socket_fds[1]), IPC::File::adopt_fd(fd_passing_socket_fds[1]) }; } Messages::RequestServer::IsSupportedProtocolResponse ConnectionFromClient::is_supported_protocol(ByteString const& protocol) @@ -110,7 +112,7 @@ void ConnectionFromClient::start_request(i32 request_id, ByteString const& metho auto id = request->id(); auto fd = request->request_fd(); m_requests.set(id, move(request)); - (void)post_message(Messages::RequestClient::RequestStarted(request_id, IPC::File(fd, IPC::File::CloseAfterSending))); + (void)post_message(Messages::RequestClient::RequestStarted(request_id, IPC::File::adopt_fd(fd))); } Messages::RequestServer::StopRequestResponse ConnectionFromClient::stop_request(i32 request_id) diff --git a/Userland/Services/WebWorker/ConnectionFromClient.cpp b/Userland/Services/WebWorker/ConnectionFromClient.cpp index 4110e4a557a..11e39a42bb4 100644 --- a/Userland/Services/WebWorker/ConnectionFromClient.cpp +++ b/Userland/Services/WebWorker/ConnectionFromClient.cpp @@ -31,7 +31,7 @@ void ConnectionFromClient::request_file(Web::FileRequest request) if (file.is_error()) handle_file_return(file.error().code(), {}, request_id); else - handle_file_return(0, IPC::File(*file.value()), request_id); + handle_file_return(0, IPC::File::adopt_file(file.release_value()), request_id); } ConnectionFromClient::ConnectionFromClient(NonnullOwnPtr socket)