From cf1fa419ab22c9e03893e8c49b0d4c48fa41cbb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Thu, 29 Dec 2022 14:57:00 +0100 Subject: [PATCH] LibThreading: Register BackgroundAction with EventLoop BackgroundActions are now added as a job to the event loop, therefore they get canceled when the loop exits and their on_complete action never runs. This fixes all UAF bugs related to BackgroundAction's use of EventLoops, as seen with e.g. thumbnail generation. --- Userland/Applications/Assistant/Providers.cpp | 2 +- .../DisplaySettings/MonitorWidget.cpp | 15 ++-- .../FileManager/PropertiesWindow.cpp | 17 ++-- .../Libraries/LibThreading/BackgroundAction.h | 77 +++++++++++++------ 4 files changed, 72 insertions(+), 39 deletions(-) diff --git a/Userland/Applications/Assistant/Providers.cpp b/Userland/Applications/Assistant/Providers.cpp index 1a0c034bca..566e0596ba 100644 --- a/Userland/Applications/Assistant/Providers.cpp +++ b/Userland/Applications/Assistant/Providers.cpp @@ -135,7 +135,7 @@ void FileProvider::query(DeprecatedString const& query, Function> results; for (auto& path : m_full_path_cache) { - if (task.is_cancelled()) + if (task.is_canceled()) return {}; auto match_result = fuzzy_match(query, path); diff --git a/Userland/Applications/DisplaySettings/MonitorWidget.cpp b/Userland/Applications/DisplaySettings/MonitorWidget.cpp index 2a9c315cc8..12b65c6b67 100644 --- a/Userland/Applications/DisplaySettings/MonitorWidget.cpp +++ b/Userland/Applications/DisplaySettings/MonitorWidget.cpp @@ -30,26 +30,25 @@ bool MonitorWidget::set_wallpaper(DeprecatedString path) if (!is_different_to_current_wallpaper_path(path)) return false; - (void)Threading::BackgroundAction>>::construct( + (void)Threading::BackgroundAction>::construct( [path](auto&) -> ErrorOr> { if (path.is_empty()) return Error::from_errno(ENOENT); return Gfx::Bitmap::load_from_file(path); }, - [this, path](ErrorOr> bitmap_or_error) -> ErrorOr { + [this, path](NonnullRefPtr bitmap) -> ErrorOr { // If we've been requested to change while we were loading the bitmap, don't bother spending the cost to // move and render the now stale bitmap. if (is_different_to_current_wallpaper_path(path)) return {}; - if (bitmap_or_error.is_error()) - m_wallpaper_bitmap = nullptr; - else - m_wallpaper_bitmap = bitmap_or_error.release_value(); + m_wallpaper_bitmap = move(bitmap); m_desktop_dirty = true; update(); - - return bitmap_or_error.is_error() ? bitmap_or_error.release_error() : ErrorOr {}; + return {}; + }, + [this, path](Error) -> void { + m_wallpaper_bitmap = nullptr; }); if (path.is_empty()) diff --git a/Userland/Applications/FileManager/PropertiesWindow.cpp b/Userland/Applications/FileManager/PropertiesWindow.cpp index e5d8392155..5c8e25878b 100644 --- a/Userland/Applications/FileManager/PropertiesWindow.cpp +++ b/Userland/Applications/FileManager/PropertiesWindow.cpp @@ -291,12 +291,12 @@ void PropertiesWindow::DirectoryStatisticsCalculator::start() VERIFY(!m_background_action); m_background_action = Threading::BackgroundAction::construct( - [this, strong_this = NonnullRefPtr(*this)](auto& task) { + [this, strong_this = NonnullRefPtr(*this)](auto& task) -> ErrorOr { auto timer = Core::ElapsedTimer(); while (!m_work_queue.is_empty()) { auto base_directory = m_work_queue.dequeue(); auto result = Core::Directory::for_each_entry(base_directory, Core::DirIterator::SkipParentAndBaseDir, [&](auto const& entry, auto const& directory) -> ErrorOr { - if (task.is_cancelled()) + if (task.is_canceled()) return Error::from_errno(ECANCELED); struct stat st = {}; @@ -315,7 +315,7 @@ void PropertiesWindow::DirectoryStatisticsCalculator::start() } // Show the first update, then show any subsequent updates every 100ms. - if (!task.is_cancelled() && on_update && (!timer.is_valid() || timer.elapsed_time() > 100_ms)) { + if (!task.is_canceled() && on_update && (!timer.is_valid() || timer.elapsed_time() > 100_ms)) { timer.start(); on_update(m_total_size_in_bytes, m_file_count, m_directory_count); } @@ -323,15 +323,18 @@ void PropertiesWindow::DirectoryStatisticsCalculator::start() return IterationDecision::Continue; }); if (result.is_error() && result.error().code() == ECANCELED) - return ECANCELED; + return Error::from_errno(ECANCELED); } - return ESUCCESS; + return 0; }, - [this](auto result) -> ErrorOr { - if (on_update && result == ESUCCESS) + [this](auto) -> ErrorOr { + if (on_update) on_update(m_total_size_in_bytes, m_file_count, m_directory_count); return {}; + }, + [](auto) { + // Ignore the error. }); } diff --git a/Userland/Libraries/LibThreading/BackgroundAction.h b/Userland/Libraries/LibThreading/BackgroundAction.h index 54c839caa3..02aa6b3408 100644 --- a/Userland/Libraries/LibThreading/BackgroundAction.h +++ b/Userland/Libraries/LibThreading/BackgroundAction.h @@ -1,7 +1,7 @@ /* * Copyright (c) 2019-2020, Sergey Bugaev * Copyright (c) 2021, Andreas Kling - * Copyright (c) 2022, the SerenityOS developers. + * Copyright (c) 2022-2023, the SerenityOS developers. * * SPDX-License-Identifier: BSD-2-Clause */ @@ -15,6 +15,7 @@ #include #include #include +#include #include namespace Threading { @@ -39,50 +40,80 @@ class BackgroundAction final : public Core::Object C_OBJECT(BackgroundAction); public: - void cancel() - { - m_cancelled = true; - } - - bool is_cancelled() const - { - return m_cancelled; - } + // Promise is an implementation detail of BackgroundAction in order to communicate with EventLoop. + // All of the promise's callbacks and state are either managed by us or by EventLoop. + using Promise = Core::Promise>; virtual ~BackgroundAction() = default; + Optional const& result() const { return m_result; } + Optional& result() { return m_result; } + + void cancel() { m_canceled = true; } + // If your action is long-running, you should periodically check the cancel state and possibly return early. + bool is_canceled() const { return m_canceled; } + private: - BackgroundAction(Function action, Function(Result)> on_complete, Optional> on_error = {}) + BackgroundAction(Function(BackgroundAction&)> action, Function(Result)> on_complete, Optional> on_error = {}) : Core::Object(&background_thread()) + , m_promise(Promise::try_create().release_value_but_fixme_should_propagate_errors()) , m_action(move(action)) , m_on_complete(move(on_complete)) { + if (m_on_complete) { + m_promise->on_resolved = [](NonnullRefPtr& object) -> ErrorOr { + auto self = static_ptr_cast>(object); + VERIFY(self->m_result.has_value()); + if (auto maybe_error = self->m_on_complete(self->m_result.value()); maybe_error.is_error()) + self->m_on_error(maybe_error.release_error()); + + return {}; + }; + Core::EventLoop::current().add_job(m_promise); + } + if (on_error.has_value()) m_on_error = on_error.release_value(); enqueue_work([this, origin_event_loop = &Core::EventLoop::current()] { - m_result = m_action(*this); - if (m_on_complete) { - origin_event_loop->deferred_invoke([this] { - auto maybe_error = m_on_complete(m_result.release_value()); - if (maybe_error.is_error()) - m_on_error(maybe_error.release_error()); - remove_from_parent(); - }); - origin_event_loop->wake(); + auto result = m_action(*this); + // The event loop cancels the promise when it exits. + m_canceled |= m_promise->is_canceled(); + // All of our work was successful and we weren't cancelled; resolve the event loop's promise. + if (!m_canceled && !result.is_error()) { + m_result = result.release_value(); + // If there is no completion callback, we don't rely on the user keeping around the event loop. + if (m_on_complete) { + origin_event_loop->deferred_invoke([this] { + // Our promise's resolution function will never error. + (void)m_promise->resolve(*this); + remove_from_parent(); + }); + origin_event_loop->wake(); + } } else { - this->remove_from_parent(); + // We were either unsuccessful or cancelled (in which case there is no error). + auto error = Error::from_errno(ECANCELED); + if (result.is_error()) + error = result.release_error(); + + m_promise->cancel(Error::from_errno(ECANCELED)); + if (m_on_error) + m_on_error(move(error)); + + remove_from_parent(); } }); } - bool m_cancelled { false }; - Function m_action; + NonnullRefPtr m_promise; + Function(BackgroundAction&)> m_action; Function(Result)> m_on_complete; Function m_on_error = [](Error error) { dbgln("Error occurred while running a BackgroundAction: {}", error); }; Optional m_result; + bool m_canceled { false }; }; }