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

@@ -135,7 +135,7 @@ void FileProvider::query(DeprecatedString const& query, Function<void(Vector<Non
             Vector<NonnullRefPtr<Result>> results;
 
             for (auto& path : m_full_path_cache) {
-                if (task.is_cancelled())
+                if (task.is_canceled())
                     return {};
 
                 auto match_result = fuzzy_match(query, path);

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<ErrorOr<NonnullRefPtr<Gfx::Bitmap>>>::construct(
+    (void)Threading::BackgroundAction<NonnullRefPtr<Gfx::Bitmap>>::construct(
         [path](auto&) -> ErrorOr<NonnullRefPtr<Gfx::Bitmap>> {
             if (path.is_empty())
                 return Error::from_errno(ENOENT);
             return Gfx::Bitmap::load_from_file(path);
         },
 
-        [this, path](ErrorOr<NonnullRefPtr<Gfx::Bitmap>> bitmap_or_error) -> ErrorOr<void> {
+        [this, path](NonnullRefPtr<Gfx::Bitmap> bitmap) -> ErrorOr<void> {
             // 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 = move(bitmap);
-                m_wallpaper_bitmap = nullptr;
-            else
-                m_wallpaper_bitmap = bitmap_or_error.release_value();
             m_desktop_dirty = true;
             update();
-
+            return {};
-            return bitmap_or_error.is_error() ? bitmap_or_error.release_error() : ErrorOr<void> {};
+        },
+        [this, path](Error) -> void {
+            m_wallpaper_bitmap = nullptr;
         });
 
     if (path.is_empty())

Userland/Applications/FileManager/PropertiesWindow.cpp

@@ -291,12 +291,12 @@ void PropertiesWindow::DirectoryStatisticsCalculator::start()
     VERIFY(!m_background_action);
 
     m_background_action = Threading::BackgroundAction<int>::construct(
-        [this, strong_this = NonnullRefPtr(*this)](auto& task) {
+        [this, strong_this = NonnullRefPtr(*this)](auto& task) -> ErrorOr<int> {
             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<IterationDecision> {
-                    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<void> {
+        [this](auto) -> ErrorOr<void> {
-            if (on_update && result == ESUCCESS)
+            if (on_update)
                 on_update(m_total_size_in_bytes, m_file_count, m_directory_count);
 
             return {};
+        },
+        [](auto) {
+            // Ignore the error.
         });
 }
 

Userland/Libraries/LibThreading/BackgroundAction.h

@@ -1,7 +1,7 @@
 /*
  * Copyright (c) 2019-2020, Sergey Bugaev <bugaevc@serenityos.org>
  * Copyright (c) 2021, Andreas Kling <kling@serenityos.org>
- * Copyright (c) 2022, the SerenityOS developers.
+ * Copyright (c) 2022-2023, the SerenityOS developers.
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -15,6 +15,7 @@
 #include <LibCore/Event.h>
 #include <LibCore/EventLoop.h>
 #include <LibCore/Object.h>
+#include <LibCore/Promise.h>
 #include <LibThreading/Thread.h>
 
 namespace Threading {
@@ -39,50 +40,80 @@ class BackgroundAction final : public Core::Object
     C_OBJECT(BackgroundAction);
 
 public:
-    void cancel()
+    // 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.
-        m_cancelled = true;
+    using Promise = Core::Promise<NonnullRefPtr<Core::Object>>;
-    }
-
-    bool is_cancelled() const
-    {
-        return m_cancelled;
-    }
 
     virtual ~BackgroundAction() = default;
 
+    Optional<Result> const& result() const { return m_result; }
+    Optional<Result>& 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<Result(BackgroundAction&)> action, Function<ErrorOr<void>(Result)> on_complete, Optional<Function<void(Error)>> on_error = {})
+    BackgroundAction(Function<ErrorOr<Result>(BackgroundAction&)> action, Function<ErrorOr<void>(Result)> on_complete, Optional<Function<void(Error)>> 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<Core::Object>& object) -> ErrorOr<void> {
+                auto self = static_ptr_cast<BackgroundAction<Result>>(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);
+            auto result = m_action(*this);
-            if (m_on_complete) {
+            // The event loop cancels the promise when it exits.
-                origin_event_loop->deferred_invoke([this] {
+            m_canceled |= m_promise->is_canceled();
-                    auto maybe_error = m_on_complete(m_result.release_value());
+            // All of our work was successful and we weren't cancelled; resolve the event loop's promise.
-                    if (maybe_error.is_error())
+            if (!m_canceled && !result.is_error()) {
-                        m_on_error(maybe_error.release_error());
+                m_result = result.release_value();
-                    remove_from_parent();
+                // 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->wake();
+                    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 };
+    NonnullRefPtr<Promise> m_promise;
-    Function<Result(BackgroundAction&)> m_action;
+    Function<ErrorOr<Result>(BackgroundAction&)> m_action;
     Function<ErrorOr<void>(Result)> m_on_complete;
     Function<void(Error)> m_on_error = [](Error error) {
         dbgln("Error occurred while running a BackgroundAction: {}", error);
     };
     Optional<Result> m_result;
+    bool m_canceled { false };
 };
 
 }