Kernel: Store Thread name as a KString

Author: https://github.com/awesomekling
Commit: https://github.com/SerenityOS/serenity/commit/d5d8fba5796

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());

Kernel/Process.cpp

@@ -735,7 +735,7 @@ KResult Process::send_signal(u8 signal, Process* sender)
     return ESRCH;
 }
 
-RefPtr<Thread> Process::create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, const String& name, u32 affinity, bool joinable)
+RefPtr<Thread> Process::create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, OwnPtr<KString> name, u32 affinity, bool joinable)
 {
     VERIFY((priority >= THREAD_PRIORITY_MIN) && (priority <= THREAD_PRIORITY_MAX));
 
@@ -746,7 +746,7 @@ RefPtr<Thread> 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)

Kernel/Process.h

@@ -185,7 +185,7 @@ public:
     static NonnullRefPtrVector<Process> all_processes();
 
     template<typename EntryFunction>
-    RefPtr<Thread> create_kernel_thread(EntryFunction entry, u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true)
+    RefPtr<Thread> create_kernel_thread(EntryFunction entry, u32 priority, OwnPtr<KString> 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<Thread> create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, const String& name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true);
+    RefPtr<Thread> create_kernel_thread(void (*entry)(void*), void* entry_data, u32 priority, OwnPtr<KString> name, u32 affinity = THREAD_AFFINITY_DEFAULT, bool joinable = true);
 
     bool is_profiling() const { return m_profiling; }
     void set_profiling(bool profiling) { m_profiling = profiling; }

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;
 }

Kernel/Syscalls/execve.cpp

@@ -634,7 +634,7 @@ KResult Process::do_exec(NonnullRefPtr<FileDescription> 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 };

Kernel/Syscalls/thread.cpp

@@ -54,7 +54,7 @@ KResultOr<FlatPtr> Process::sys$create_thread(void* (*entry)(void*), Userspace<c
     // So give it a unique name until the user calls $set_thread_name on it
     // length + 4 to give space for our extra junk at the end
     StringBuilder builder(m_name.length() + 4);
-    thread->set_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<FlatPtr> Process::sys$set_thread_name(pid_t tid, Userspace<const char*
 {
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this)
     REQUIRE_PROMISE(stdio);
-    auto name = copy_string_from_user(user_name, user_name_length);
-    if (name.is_null())
-        return EFAULT;
+
+    auto name_or_error = try_copy_kstring_from_user(user_name, user_name_length);
+    if (name_or_error.is_error())
+        return name_or_error.error();
+    auto name = name_or_error.release_value();
 
     const size_t max_thread_name_size = 64;
-    if (name.length() > max_thread_name_size)
+    if (name->length() > max_thread_name_size)
         return EINVAL;
 
     auto thread = Thread::from_tid(tid);
@@ -213,12 +215,20 @@ KResultOr<FlatPtr> Process::sys$get_thread_name(pid_t tid, Userspace<char*> 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;
 }

Kernel/Thread.cpp

@@ -52,18 +52,20 @@ KResultOr<NonnullRefPtr<Thread>> Thread::try_create(NonnullRefPtr<Process> 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> process, NonnullOwnPtr<Region> kernel_stack_region, NonnullRefPtr<Timer> block_timer, NonnullOwnPtr<FPUState> fpu_state)
+Thread::Thread(NonnullRefPtr<Process> process, NonnullOwnPtr<Region> kernel_stack_region, NonnullRefPtr<Timer> block_timer, NonnullOwnPtr<FPUState> fpu_state, OwnPtr<KString> 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);
     };

Kernel/Thread.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2018-2021, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -26,6 +26,7 @@
 #include <Kernel/FileSystem/InodeIdentifier.h>
 #include <Kernel/Forward.h>
 #include <Kernel/KResult.h>
+#include <Kernel/KString.h>
 #include <Kernel/LockMode.h>
 #include <Kernel/Scheduler.h>
 #include <Kernel/TimerQueue.h>
@@ -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<KString> name)
     {
         ScopedSpinLock lock(m_lock);
         m_name = move(name);
@@ -1218,7 +1215,7 @@ public:
     String backtrace();
 
 private:
-    Thread(NonnullRefPtr<Process>, NonnullOwnPtr<Region>, NonnullRefPtr<Timer>, NonnullOwnPtr<FPUState>);
+    Thread(NonnullRefPtr<Process>, NonnullOwnPtr<Region>, NonnullRefPtr<Timer>, NonnullOwnPtr<FPUState>, OwnPtr<KString>);
 
     IntrusiveListNode<Thread> m_process_thread_list_node;
     int m_runnable_priority { -1 };
@@ -1349,7 +1346,7 @@ private:
 
     OwnPtr<FPUState> m_fpu_state;
     State m_state { Invalid };
-    String m_name;
+    OwnPtr<KString> m_name;
     u32 m_priority { THREAD_PRIORITY_NORMAL };
 
     State m_stop_state { Invalid };