From d5d8fba579666338cd928ac3ebe2270fded54287 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Thu, 5 Aug 2021 22:22:26 +0200 Subject: [PATCH] Kernel: Store Thread name as a KString --- Kernel/GlobalProcessExposed.cpp | 1 + Kernel/Process.cpp | 4 ++-- Kernel/Process.h | 6 +++--- Kernel/Scheduler.cpp | 4 ++-- Kernel/Syscalls/execve.cpp | 2 +- Kernel/Syscalls/thread.cpp | 24 +++++++++++++++++------- Kernel/Thread.cpp | 10 ++++++---- Kernel/Thread.h | 25 +++++++++++-------------- 8 files changed, 43 insertions(+), 33 deletions(-) diff --git a/Kernel/GlobalProcessExposed.cpp b/Kernel/GlobalProcessExposed.cpp index 9d31521f70..5fc351509d 100644 --- a/Kernel/GlobalProcessExposed.cpp +++ b/Kernel/GlobalProcessExposed.cpp @@ -464,6 +464,7 @@ private: process_object.add("kernel", process.is_kernel_process()); auto thread_array = process_object.add_array("threads"); process.for_each_thread([&](const Thread& thread) { + ScopedSpinLock locker(thread.get_lock()); auto thread_object = thread_array.add_object(); #if LOCK_DEBUG thread_object.add("lock_count", thread.lock_count()); diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 076e439751..64a5e37620 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -735,7 +735,7 @@ KResult Process::send_signal(u8 signal, Process* sender) return ESRCH; } -RefPtr Process::create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, const String& name, u32 affinity, bool joinable) +RefPtr Process::create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, OwnPtr name, u32 affinity, bool joinable) { VERIFY((priority >= THREAD_PRIORITY_MIN) && (priority <= THREAD_PRIORITY_MAX)); @@ -746,7 +746,7 @@ RefPtr Process::create_kernel_thread(void (*entry)(void*), void* entry_d return {}; auto thread = thread_or_error.release_value(); - thread->set_name(name); + thread->set_name(move(name)); thread->set_affinity(affinity); thread->set_priority(priority); if (!joinable) diff --git a/Kernel/Process.h b/Kernel/Process.h index 728c353201..ef9dc2a5d4 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -185,7 +185,7 @@ public: static NonnullRefPtrVector all_processes(); template - RefPtr create_kernel_thread(EntryFunction entry, u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true) + RefPtr create_kernel_thread(EntryFunction entry, u32 priority, OwnPtr name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true) { auto* entry_func = new EntryFunction(move(entry)); return create_kernel_thread([](void* data) { @@ -193,9 +193,9 @@ public: (*func)(); delete func; }, - priority, name, affinity, joinable); + priority, move(name), affinity, joinable); } - RefPtr create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true); + RefPtr create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, OwnPtr name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true); bool is_profiling() const { return m_profiling; } void set_profiling(bool profiling) { m_profiling = profiling; } diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index 2da03253b2..f84415d26d 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -424,7 +424,7 @@ UNMAP_AFTER_INIT void Scheduler::initialize() VERIFY(s_colonel_process); VERIFY(idle_thread); idle_thread->set_priority(THREAD_PRIORITY_MIN); - idle_thread->set_name(StringView("idle thread #0")); + idle_thread->set_name(KString::try_create("idle thread #0")); set_idle_thread(idle_thread); } @@ -443,7 +443,7 @@ UNMAP_AFTER_INIT Thread* Scheduler::create_ap_idle_thread(u32 cpu) VERIFY(Processor::is_bootstrap_processor()); VERIFY(s_colonel_process); - Thread* idle_thread = s_colonel_process->create_kernel_thread(idle_loop, nullptr, THREAD_PRIORITY_MIN, String::formatted("idle thread #{}", cpu), 1 << cpu, false); + Thread* idle_thread = s_colonel_process->create_kernel_thread(idle_loop, nullptr, THREAD_PRIORITY_MIN, KString::try_create(String::formatted("idle thread #{}", cpu)), 1 << cpu, false); VERIFY(idle_thread); return idle_thread; } diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index e96038e8f4..6412717a8d 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -634,7 +634,7 @@ KResult Process::do_exec(NonnullRefPtr main_program_description // NOTE: Be careful to not trigger any page faults below! m_name = parts.take_last(); - new_main_thread->set_name(m_name); + new_main_thread->set_name(KString::try_create(m_name)); { ProtectedDataMutationScope scope { *this }; diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index b896c0997d..a26e7f27a0 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -54,7 +54,7 @@ KResultOr Process::sys$create_thread(void* (*entry)(void*), Userspaceset_name(String::formatted("{} [{}]", m_name, thread->tid().value())); + thread->set_name(KString::try_create(String::formatted("{} [{}]", m_name, thread->tid().value()))); if (!is_thread_joinable) thread->detach(); @@ -186,12 +186,14 @@ KResultOr Process::sys$set_thread_name(pid_t tid, Userspace max_thread_name_size) + if (name->length() > max_thread_name_size) return EINVAL; auto thread = Thread::from_tid(tid); @@ -213,12 +215,20 @@ KResultOr Process::sys$get_thread_name(pid_t tid, Userspace buff if (!thread || thread->pid() != pid()) return ESRCH; - // We must make a temporary copy here to avoid a race with sys$set_thread_name + ScopedSpinLock locker(thread->get_lock()); auto thread_name = thread->name(); + + if (thread_name.is_null()) { + char null_terminator = '\0'; + if (!copy_to_user(buffer, &null_terminator, sizeof(null_terminator))) + return EFAULT; + return 0; + } + if (thread_name.length() + 1 > buffer_size) return ENAMETOOLONG; - if (!copy_to_user(buffer, thread_name.characters(), thread_name.length() + 1)) + if (!copy_to_user(buffer, thread_name.characters_without_null_termination(), thread_name.length() + 1)) return EFAULT; return 0; } diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index fadc32d026..39d1d4b940 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -52,18 +52,20 @@ KResultOr> Thread::try_create(NonnullRefPtr proce if (!block_timer) return ENOMEM; - auto thread = adopt_ref_if_nonnull(new (nothrow) Thread(move(process), kernel_stack_region.release_nonnull(), block_timer.release_nonnull(), fpu_state.release_nonnull())); + auto name = KString::try_create(process->name()); + + auto thread = adopt_ref_if_nonnull(new (nothrow) Thread(move(process), kernel_stack_region.release_nonnull(), block_timer.release_nonnull(), fpu_state.release_nonnull(), move(name))); if (!thread) return ENOMEM; return thread.release_nonnull(); } -Thread::Thread(NonnullRefPtr process, NonnullOwnPtr kernel_stack_region, NonnullRefPtr block_timer, NonnullOwnPtr fpu_state) +Thread::Thread(NonnullRefPtr process, NonnullOwnPtr kernel_stack_region, NonnullRefPtr block_timer, NonnullOwnPtr fpu_state, OwnPtr name) : m_process(move(process)) , m_kernel_stack_region(move(kernel_stack_region)) , m_fpu_state(move(fpu_state)) - , m_name(m_process->name()) + , m_name(move(name)) , m_block_timer(block_timer) , m_global_procfs_inode_index(ProcFSComponentRegistry::the().allocate_inode_index()) { @@ -1020,7 +1022,7 @@ DispatchSignalResult Thread::dispatch_signal(u8 signal) push_value_on_user_stack(stack, signal); push_value_on_user_stack(stack, handler_vaddr.get()); - push_value_on_user_stack(stack, 0); //push fake return address + push_value_on_user_stack(stack, 0); // push fake return address VERIFY((*stack % 16) == 0); }; diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 0bfdbf6f76..1194040e00 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018-2020, Andreas Kling + * Copyright (c) 2018-2021, Andreas Kling * * SPDX-License-Identifier: BSD-2-Clause */ @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -178,19 +179,15 @@ public: Process& process() { return m_process; } const Process& process() const { return m_process; } - String name() const + // NOTE: This returns a null-terminated string. + StringView name() const { - // Because the name can be changed, we can't return a const - // reference here. We must make a copy - ScopedSpinLock lock(m_lock); - return m_name; + // NOTE: Whoever is calling this needs to be holding our lock while reading the name. + VERIFY(m_lock.own_lock()); + return m_name ? m_name->view() : StringView {}; } - void set_name(const StringView& s) - { - ScopedSpinLock lock(m_lock); - m_name = s; - } - void set_name(String&& name) + + void set_name(OwnPtr name) { ScopedSpinLock lock(m_lock); m_name = move(name); @@ -1218,7 +1215,7 @@ public: String backtrace(); private: - Thread(NonnullRefPtr, NonnullOwnPtr, NonnullRefPtr, NonnullOwnPtr); + Thread(NonnullRefPtr, NonnullOwnPtr, NonnullRefPtr, NonnullOwnPtr, OwnPtr); IntrusiveListNode m_process_thread_list_node; int m_runnable_priority { -1 }; @@ -1349,7 +1346,7 @@ private: OwnPtr m_fpu_state; State m_state { Invalid }; - String m_name; + OwnPtr m_name; u32 m_priority { THREAD_PRIORITY_NORMAL }; State m_stop_state { Invalid };