diff options
author | Andreas Kling <kling@serenityos.org> | 2023-04-04 16:38:46 +0200 |
---|---|---|
committer | Andreas Kling <kling@serenityos.org> | 2023-04-05 11:37:27 +0200 |
commit | 3e30d9bc99728f4d4b6e877abb899502873815af (patch) | |
tree | 42ac28129740011dc2b16ef59840c7514bfa77d8 /Kernel | |
parent | 37bfc36601c5355e3dbac53c23c735d9c089eb54 (diff) | |
download | serenity-3e30d9bc99728f4d4b6e877abb899502873815af.zip |
Kernel: Make ProcessGroup a ListedRefCounted and fix two races
This closes two race windows:
- ProcessGroup removed itself from the "all process groups" list in its
destructor. It was possible to walk the list between the last unref()
and the destructor invocation, and grab a pointer to a ProcessGroup
that was about to get deleted.
- sys$setsid() could end up creating a process group that already
existed, as there was a race window between checking if the PGID
is used, and actually creating a ProcessGroup with that PGID.
Diffstat (limited to 'Kernel')
-rw-r--r-- | Kernel/ProcessGroup.cpp | 37 | ||||
-rw-r--r-- | Kernel/ProcessGroup.h | 16 | ||||
-rw-r--r-- | Kernel/Syscalls/setpgid.cpp | 12 |
3 files changed, 29 insertions, 36 deletions
diff --git a/Kernel/ProcessGroup.cpp b/Kernel/ProcessGroup.cpp index be7b389663..2e28beda95 100644 --- a/Kernel/ProcessGroup.cpp +++ b/Kernel/ProcessGroup.cpp @@ -1,6 +1,6 @@ /* * Copyright (c) 2020, the SerenityOS developers. - * Copyright (c) 2021, Andreas Kling <kling@serenityos.org> + * Copyright (c) 2021-2023, Andreas Kling <kling@serenityos.org> * * SPDX-License-Identifier: BSD-2-Clause */ @@ -10,45 +10,44 @@ namespace Kernel { -static Singleton<SpinlockProtected<ProcessGroup::List, LockRank::None>> s_process_groups; +static Singleton<SpinlockProtected<ProcessGroup::AllInstancesList, LockRank::None>> s_all_instances; -SpinlockProtected<ProcessGroup::List, LockRank::None>& process_groups() +SpinlockProtected<ProcessGroup::AllInstancesList, LockRank::None>& ProcessGroup::all_instances() { - return *s_process_groups; + return s_all_instances; } -ProcessGroup::~ProcessGroup() -{ - process_groups().with([&](auto& groups) { - groups.remove(*this); - }); -} +ProcessGroup::~ProcessGroup() = default; -ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::create(ProcessGroupID pgid) +ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::create_if_unused_pgid(ProcessGroupID pgid) { - auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid))); - process_groups().with([&](auto& groups) { - groups.prepend(*process_group); + return all_instances().with([&](auto& all_instances) -> ErrorOr<NonnullRefPtr<ProcessGroup>> { + for (auto& process_group : all_instances) { + if (process_group.pgid() == pgid) + return EPERM; + } + auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid))); + all_instances.prepend(*process_group); + return process_group; }); - return process_group; } ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::find_or_create(ProcessGroupID pgid) { - return process_groups().with([&](auto& groups) -> ErrorOr<NonnullRefPtr<ProcessGroup>> { - for (auto& group : groups) { + return all_instances().with([&](auto& all_instances) -> ErrorOr<NonnullRefPtr<ProcessGroup>> { + for (auto& group : all_instances) { if (group.pgid() == pgid) return group; } auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid))); - groups.prepend(*process_group); + all_instances.prepend(*process_group); return process_group; }); } RefPtr<ProcessGroup> ProcessGroup::from_pgid(ProcessGroupID pgid) { - return process_groups().with([&](auto& groups) -> RefPtr<ProcessGroup> { + return all_instances().with([&](auto& groups) -> RefPtr<ProcessGroup> { for (auto& group : groups) { if (group.pgid() == pgid) return &group; diff --git a/Kernel/ProcessGroup.h b/Kernel/ProcessGroup.h index 217316347c..2f74ff8d79 100644 --- a/Kernel/ProcessGroup.h +++ b/Kernel/ProcessGroup.h @@ -1,15 +1,15 @@ /* - * Copyright (c) 2020, the SerenityOS developers. + * Copyright (c) 2020-2023, the SerenityOS developers. * * SPDX-License-Identifier: BSD-2-Clause */ #pragma once -#include <AK/AtomicRefCounted.h> #include <AK/IntrusiveList.h> #include <AK/RefPtr.h> #include <Kernel/Forward.h> +#include <Kernel/Library/ListedRefCounted.h> #include <Kernel/Library/LockWeakable.h> #include <Kernel/Locking/SpinlockProtected.h> #include <Kernel/UnixTypes.h> @@ -17,7 +17,7 @@ namespace Kernel { class ProcessGroup - : public AtomicRefCounted<ProcessGroup> + : public ListedRefCounted<ProcessGroup, LockType::Spinlock> , public LockWeakable<ProcessGroup> { AK_MAKE_NONMOVABLE(ProcessGroup); @@ -26,7 +26,7 @@ class ProcessGroup public: ~ProcessGroup(); - static ErrorOr<NonnullRefPtr<ProcessGroup>> create(ProcessGroupID); + static ErrorOr<NonnullRefPtr<ProcessGroup>> create_if_unused_pgid(ProcessGroupID); static ErrorOr<NonnullRefPtr<ProcessGroup>> find_or_create(ProcessGroupID); static RefPtr<ProcessGroup> from_pgid(ProcessGroupID); @@ -38,13 +38,13 @@ private: { } - IntrusiveListNode<ProcessGroup> m_list_node; ProcessGroupID m_pgid; + mutable IntrusiveListNode<ProcessGroup> m_list_node; + public: - using List = IntrusiveList<&ProcessGroup::m_list_node>; + using AllInstancesList = IntrusiveList<&ProcessGroup::m_list_node>; + static SpinlockProtected<AllInstancesList, LockRank::None>& all_instances(); }; -SpinlockProtected<ProcessGroup::List, LockRank::None>& process_groups(); - } diff --git a/Kernel/Syscalls/setpgid.cpp b/Kernel/Syscalls/setpgid.cpp index 32f908cf7e..bfc95acde8 100644 --- a/Kernel/Syscalls/setpgid.cpp +++ b/Kernel/Syscalls/setpgid.cpp @@ -28,16 +28,10 @@ ErrorOr<FlatPtr> Process::sys$setsid() { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); TRY(require_promise(Pledge::proc)); - bool found_process_with_same_pgid_as_my_pid = false; - TRY(Process::for_each_in_pgrp_in_same_jail(pid().value(), [&](auto&) -> ErrorOr<void> { - found_process_with_same_pgid_as_my_pid = true; - return {}; - })); - if (found_process_with_same_pgid_as_my_pid) - return EPERM; - // Create a new Session and a new ProcessGroup. - auto process_group = TRY(ProcessGroup::create(ProcessGroupID(pid().value()))); + // NOTE: ProcessGroup::create_if_unused_pgid() will fail with EPERM + // if a process group with the same PGID already exists. + auto process_group = TRY(ProcessGroup::create_if_unused_pgid(ProcessGroupID(pid().value()))); return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr<FlatPtr> { protected_data.tty = nullptr; protected_data.process_group = move(process_group); |