Fix signal subscription leak in xdg-desktop-portal callbacks Prevent duplicate signal processing from a misbehaving portal by unsubscribing each response signal handler on first invocation. Also unsubscribe the closed signal handler before re-registering. Bug: chromium:505371989 Change-Id: Ie5403c46d84aa5f13a655ad4cc618bb9402acbd6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/466082 Reviewed-by: Mark Foltz <mfoltz@chromium.org> Commit-Queue: Mark Foltz <mfoltz@chromium.org> Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47559}
diff --git a/modules/desktop_capture/linux/wayland/screen_capture_portal_interface.cc b/modules/desktop_capture/linux/wayland/screen_capture_portal_interface.cc index 3a71218..40ef29f 100644 --- a/modules/desktop_capture/linux/wayland/screen_capture_portal_interface.cc +++ b/modules/desktop_capture/linux/wayland/screen_capture_portal_interface.cc
@@ -100,6 +100,12 @@ return; } + if (UnsubscribeSignalHandler(connection, session_closed_signal_id)) { + RTC_LOG(LS_ERROR) << "Duplicate session closed signal registration."; + OnPortalDone(RequestResponse::kError); + return; + } + session_closed_signal_id = g_dbus_connection_signal_subscribe( connection, kDesktopBusName, kSessionInterfaceName, /*member=*/"Closed", session_handle.c_str(), /*arg0=*/nullptr, G_DBUS_SIGNAL_FLAGS_NONE,
diff --git a/modules/desktop_capture/linux/wayland/screencast_portal.cc b/modules/desktop_capture/linux/wayland/screencast_portal.cc index e399d38..ed35bfd 100644 --- a/modules/desktop_capture/linux/wayland/screencast_portal.cc +++ b/modules/desktop_capture/linux/wayland/screencast_portal.cc
@@ -41,6 +41,7 @@ using xdg_portal::SetupSessionRequestHandlers; using xdg_portal::StartSessionRequest; using xdg_portal::TearDownSession; +using xdg_portal::UnsubscribeSignalHandler; } // namespace @@ -113,18 +114,11 @@ } // static -void UnsubscribeSignalHandler(GDBusConnection* connection, guint* signal_id) { - if (signal_id && *signal_id) { - g_dbus_connection_signal_unsubscribe(connection, *signal_id); - *signal_id = 0; - } -} - void ScreenCastPortal::UnsubscribeSignalHandlers() { - UnsubscribeSignalHandler(connection_, &session_request_signal_id_); - UnsubscribeSignalHandler(connection_, &sources_request_signal_id_); - UnsubscribeSignalHandler(connection_, &start_request_signal_id_); - UnsubscribeSignalHandler(connection_, &session_closed_signal_id_); + UnsubscribeSignalHandler(connection_, session_request_signal_id_); + UnsubscribeSignalHandler(connection_, sources_request_signal_id_); + UnsubscribeSignalHandler(connection_, start_request_signal_id_); + UnsubscribeSignalHandler(connection_, session_closed_signal_id_); } void ScreenCastPortal::SetSessionDetails( @@ -205,6 +199,13 @@ if (!that) return; + if (!UnsubscribeSignalHandler(that->connection_, + that->session_request_signal_id_)) { + RTC_LOG(LS_ERROR) << "Duplicate session request signal from portal."; + that->OnPortalDone(RequestResponse::kError); + return; + } + that->RegisterSessionClosedSignalHandler( OnSessionClosedSignal, parameters, that->connection_, that->session_handle_, that->session_closed_signal_id_, that->guard_); @@ -230,6 +231,12 @@ if (!that) return; + if (!UnsubscribeSignalHandler(that->connection_, + that->session_closed_signal_id_)) { + RTC_LOG(LS_ERROR) << "Duplicate session closed signal from portal."; + return; + } + RTC_LOG(LS_INFO) << "Received closed signal from session."; // Clear the session handle to avoid calling Session::Close from the destructor @@ -328,7 +335,7 @@ if (!handle) { RTC_LOG(LS_ERROR) << "Failed to initialize the screen cast session."; UnsubscribeSignalHandler(that->connection_, - &that->sources_request_signal_id_); + that->sources_request_signal_id_); that->OnPortalDone(RequestResponse::kError); return; } @@ -350,6 +357,13 @@ if (!that) return; + if (!UnsubscribeSignalHandler(that->connection_, + that->sources_request_signal_id_)) { + RTC_LOG(LS_ERROR) << "Duplicate sources signal from portal."; + that->OnPortalDone(RequestResponse::kError); + return; + } + RTC_LOG(LS_INFO) << "Received sources signal from session."; uint32_t portal_response; @@ -392,6 +406,13 @@ if (!that) return; + if (!UnsubscribeSignalHandler(that->connection_, + that->start_request_signal_id_)) { + RTC_LOG(LS_ERROR) << "Duplicate start signal from portal."; + that->OnPortalDone(RequestResponse::kError); + return; + } + RTC_LOG(LS_INFO) << "Start signal received."; uint32_t portal_response; Scoped<GVariant> response_data;
diff --git a/modules/portal/xdg_desktop_portal_utils.cc b/modules/portal/xdg_desktop_portal_utils.cc index 723978c..88f7ca1 100644 --- a/modules/portal/xdg_desktop_portal_utils.cc +++ b/modules/portal/xdg_desktop_portal_utils.cc
@@ -73,6 +73,14 @@ return webrtc::StrJoin(parts, "/"); } +bool UnsubscribeSignalHandler(GDBusConnection* connection, guint& signal_id) { + if (!signal_id) + return false; + g_dbus_connection_signal_unsubscribe(connection, signal_id); + signal_id = 0; + return true; +} + uint32_t SetupRequestResponseSignal(absl::string_view object_path, const GDBusSignalCallback callback, scoped_refptr<PortalGuard> guard,
diff --git a/modules/portal/xdg_desktop_portal_utils.h b/modules/portal/xdg_desktop_portal_utils.h index ab9ddb3..28d7eea 100644 --- a/modules/portal/xdg_desktop_portal_utils.h +++ b/modules/portal/xdg_desktop_portal_utils.h
@@ -62,6 +62,11 @@ RTC_EXPORT std::string PrepareSignalHandle(absl::string_view token, GDBusConnection* connection); +// Unsubscribes a signal subscription and zeros its ID. +// Returns true if the subscription was active, false if the ID was already 0. +RTC_EXPORT bool UnsubscribeSignalHandler(GDBusConnection* connection, + guint& signal_id); + // Sets up the callback to execute when a response signal is received for the // given object. Adds a ref to the guard for the signal subscription. RTC_EXPORT uint32_t
diff --git a/modules/video_capture/linux/camera_portal.cc b/modules/video_capture/linux/camera_portal.cc index 5a9c88b..89aa3fd 100644 --- a/modules/video_capture/linux/camera_portal.cc +++ b/modules/video_capture/linux/camera_portal.cc
@@ -92,11 +92,7 @@ guard_->portal = nullptr; } - if (access_request_signal_id_) { - g_dbus_connection_signal_unsubscribe(connection_, - access_request_signal_id_); - access_request_signal_id_ = 0; - } + xdg_portal::UnsubscribeSignalHandler(connection_, access_request_signal_id_); if (cancellable_) { g_object_unref(cancellable_); cancellable_ = nullptr; @@ -182,11 +178,8 @@ if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; RTC_LOG(LS_ERROR) << "Failed to access portal:" << error->message; - if (that->access_request_signal_id_) { - g_dbus_connection_signal_unsubscribe(that->connection_, - that->access_request_signal_id_); - that->access_request_signal_id_ = 0; - } + xdg_portal::UnsubscribeSignalHandler(that->connection_, + that->access_request_signal_id_); that->OnPortalDone(RequestResponse::kError); } } @@ -204,6 +197,13 @@ if (!that) return; + if (!xdg_portal::UnsubscribeSignalHandler(that->connection_, + that->access_request_signal_id_)) { + RTC_LOG(LS_ERROR) << "Duplicate access response signal from portal."; + that->OnPortalDone(RequestResponse::kError); + return; + } + uint32_t portal_response; g_variant_get(parameters, "(u@a{sv})", &portal_response, nullptr); if (portal_response) { @@ -240,11 +240,8 @@ if (g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; RTC_LOG(LS_ERROR) << "Failed to open PipeWire remote:" << error->message; - if (that->access_request_signal_id_) { - g_dbus_connection_signal_unsubscribe(that->connection_, - that->access_request_signal_id_); - that->access_request_signal_id_ = 0; - } + xdg_portal::UnsubscribeSignalHandler(that->connection_, + that->access_request_signal_id_); that->OnPortalDone(RequestResponse::kError); return; }