Kernel: Fix kernel null deref on process crash during join_thread()

The join_thread() syscall is not supposed to be interruptible by
signals, but it was. And since the process death mechanism piggybacked
on signal interrupts, it was possible to interrupt a pthread_join() by
killing the process that was doing it, leading to confusing due to some
assumptions being made by Thread::finalize() for threads that have a
pending joiner.

This patch fixes the issue by making "interrupted by death" a distinct
block result separate from "interrupted by signal". Then we handle that
state in join_thread() and tidy things up so that thread finalization
doesn't get confused by the pending joiner being gone.

Test: Tests/Kernel/null-deref-crash-during-pthread_join.cpp

7 files changed, 55 insertions, 20 deletions

diff --git a/Kernel/Net/IPv4Socket.cpp b/Kernel/Net/IPv4Socket.cpp
index ae4ceb136c..d4d84664e7 100644
--- a/Kernel/Net/IPv4Socket.cpp
+++ b/Kernel/Net/IPv4Socket.cpp
@@ -240,7 +240,7 @@ ssize_t IPv4Socket::recvfrom(FileDescription& description, void* buffer, size_t
 
             LOCKER(lock());
             if (!m_can_read) {
-                if (res == Thread::BlockResult::InterruptedBySignal)
+                if (res != Thread::BlockResult::WokeNormally)
                     return -EINTR;
 
                 // Unblocked due to timeout.
@@ -288,7 +288,7 @@ ssize_t IPv4Socket::recvfrom(FileDescription& description, void* buffer, size_t
 
         LOCKER(lock());
         if (!m_can_read) {
-            if (res == Thread::BlockResult::InterruptedBySignal)
+            if (res != Thread::BlockResult::WokeNormally)
                 return -EINTR;
 
             // Unblocked due to timeout.
diff --git a/Kernel/Net/LocalSocket.cpp b/Kernel/Net/LocalSocket.cpp
index b083b553be..07e10e769a 100644
--- a/Kernel/Net/LocalSocket.cpp
+++ b/Kernel/Net/LocalSocket.cpp
@@ -143,7 +143,7 @@ KResult LocalSocket::connect(FileDescription& description, const sockaddr* addre
         return KSuccess;
     }
 
-    if (current->block<Thread::ConnectBlocker>(description) == Thread::BlockResult::InterruptedBySignal) {
+    if (current->block<Thread::ConnectBlocker>(description) != Thread::BlockResult::WokeNormally) {
         m_connect_side_role = Role::None;
         return KResult(-EINTR);
     }
@@ -268,7 +268,7 @@ ssize_t LocalSocket::recvfrom(FileDescription& description, void* buffer, size_t
         }
     } else if (!can_read(description)) {
         auto result = current->block<Thread::ReceiveBlocker>(description);
-        if (result == Thread::BlockResult::InterruptedBySignal)
+        if (result != Thread::BlockResult::WokeNormally)
             return -EINTR;
     }
     if (!has_attached_peer(description) && buffer_for_me.is_empty())
diff --git a/Kernel/Net/TCPSocket.cpp b/Kernel/Net/TCPSocket.cpp
index 720b46368d..e6325b6484 100644
--- a/Kernel/Net/TCPSocket.cpp
+++ b/Kernel/Net/TCPSocket.cpp
@@ -341,7 +341,7 @@ KResult TCPSocket::protocol_connect(FileDescription& description, ShouldBlock sh
     m_direction = Direction::Outgoing;
 
     if (should_block == ShouldBlock::Yes) {
-        if (current->block<Thread::ConnectBlocker>(description) == Thread::BlockResult::InterruptedBySignal)
+        if (current->block<Thread::ConnectBlocker>(description) != Thread::BlockResult::WokeNormally)
             return KResult(-EINTR);
         ASSERT(setup_state() == SetupState::Completed);
         if (has_error()) {
diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp
index e3ec87a198..5bd8de15e6 100644
--- a/Kernel/Process.cpp
+++ b/Kernel/Process.cpp
@@ -1369,7 +1369,7 @@ ssize_t Process::do_write(FileDescription& description, const u8* data, int data
 #ifdef IO_DEBUG
             dbgprintf("block write on %d\n", fd);
 #endif
-            if (current->block<Thread::WriteBlocker>(description) == Thread::BlockResult::InterruptedBySignal) {
+            if (current->block<Thread::WriteBlocker>(description) != Thread::BlockResult::WokeNormally) {
                 if (nwritten == 0)
                     return -EINTR;
             }
@@ -1430,7 +1430,7 @@ ssize_t Process::sys$read(int fd, u8* buffer, ssize_t size)
         return -EISDIR;
     if (description->is_blocking()) {
         if (!description->can_read()) {
-            if (current->block<Thread::ReadBlocker>(*description) == Thread::BlockResult::InterruptedBySignal)
+            if (current->block<Thread::ReadBlocker>(*description) != Thread::BlockResult::WokeNormally)
                 return -EINTR;
         }
     }
@@ -2042,7 +2042,7 @@ pid_t Process::sys$waitpid(pid_t waitee, int* wstatus, int options)
     }
 
     pid_t waitee_pid = waitee;
-    if (current->block<Thread::WaitBlocker>(options, waitee_pid) == Thread::BlockResult::InterruptedBySignal)
+    if (current->block<Thread::WaitBlocker>(options, waitee_pid) != Thread::BlockResult::WokeNormally)
         return -EINTR;
 
     InterruptDisabler disabler;
@@ -2445,7 +2445,7 @@ int Process::sys$select(const Syscall::SC_select_params* params)
 #endif
 
     if (!timeout || select_has_timeout) {
-        if (current->block<Thread::SelectBlocker>(computed_timeout, select_has_timeout, rfds, wfds, efds) == Thread::BlockResult::InterruptedBySignal)
+        if (current->block<Thread::SelectBlocker>(computed_timeout, select_has_timeout, rfds, wfds, efds) != Thread::BlockResult::WokeNormally)
             return -EINTR;
     }
 
@@ -2505,7 +2505,7 @@ int Process::sys$poll(pollfd* fds, int nfds, int timeout)
 #endif
 
     if (has_timeout || timeout < 0) {
-        if (current->block<Thread::SelectBlocker>(actual_timeout, has_timeout, rfds, wfds, Thread::SelectBlocker::FDVector()) == Thread::BlockResult::InterruptedBySignal)
+        if (current->block<Thread::SelectBlocker>(actual_timeout, has_timeout, rfds, wfds, Thread::SelectBlocker::FDVector()) != Thread::BlockResult::WokeNormally)
             return -EINTR;
     }
 
@@ -2811,7 +2811,7 @@ int Process::sys$accept(int accepting_socket_fd, sockaddr* address, socklen_t* a
     auto& socket = *accepting_socket_description->socket();
     if (!socket.can_accept()) {
         if (accepting_socket_description->is_blocking()) {
-            if (current->block<Thread::AcceptBlocker>(*accepting_socket_description) == Thread::BlockResult::InterruptedBySignal)
+            if (current->block<Thread::AcceptBlocker>(*accepting_socket_description) != Thread::BlockResult::WokeNormally)
                 return -EINTR;
         } else {
             return -EAGAIN;
@@ -3358,9 +3358,18 @@ int Process::sys$join_thread(int tid, void** exit_value)
 
     void* joinee_exit_value = nullptr;
 
-    // FIXME: pthread_join() should not be interruptable. Enforce this somehow?
-    auto result = current->block<Thread::JoinBlocker>(*thread, joinee_exit_value);
-    (void)result;
+    // NOTE: pthread_join() cannot be interrupted by signals. Only by death.
+    for (;;) {
+        auto result = current->block<Thread::JoinBlocker>(*thread, joinee_exit_value);
+        if (result == Thread::BlockResult::InterruptedByDeath) {
+            // NOTE: This cleans things up so that Thread::finalize() won't
+            //       get confused about a missing joiner when finalizing the joinee.
+            InterruptDisabler disabler;
+            current->m_joinee->m_joiner = nullptr;
+            current->m_joinee = nullptr;
+            return 0;
+        }
+    }
 
     // NOTE: 'thread' is very possibly deleted at this point. Clear it just to be safe.
     thread = nullptr;
diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp
index d957fdb952..e516d25e15 100644
--- a/Kernel/Thread.cpp
+++ b/Kernel/Thread.cpp
@@ -155,10 +155,8 @@ void Thread::set_should_die()
     if (is_blocked()) {
         ASSERT(in_kernel());
         ASSERT(m_blocker != nullptr);
-        // We're blocked in the kernel. Pretend to have
-        // been interrupted by a signal (perhaps that is
-        // what has actually killed us).
-        m_blocker->set_interrupted_by_signal();
+        // We're blocked in the kernel.
+        m_blocker->set_interrupted_by_death();
         unblock();
     } else if (!in_kernel()) {
         // We're executing in userspace (and we're clearly
@@ -209,7 +207,7 @@ u64 Thread::sleep(u32 ticks)
     u64 wakeup_time = g_uptime + ticks;
     auto ret = current->block<Thread::SleepBlocker>(wakeup_time);
     if (wakeup_time > g_uptime) {
-        ASSERT(ret == Thread::BlockResult::InterruptedBySignal);
+        ASSERT(ret != Thread::BlockResult::WokeNormally);
     }
     return wakeup_time;
 }
@@ -219,7 +217,7 @@ u64 Thread::sleep_until(u64 wakeup_time)
     ASSERT(state() == Thread::Running);
     auto ret = current->block<Thread::SleepBlocker>(wakeup_time);
     if (wakeup_time > g_uptime)
-        ASSERT(ret == Thread::BlockResult::InterruptedBySignal);
+        ASSERT(ret != Thread::BlockResult::WokeNormally);
     return wakeup_time;
 }
 
diff --git a/Kernel/Thread.h b/Kernel/Thread.h
index b75b8d63d7..c17b43bbba 100644
--- a/Kernel/Thread.h
+++ b/Kernel/Thread.h
@@ -99,11 +99,14 @@ public:
         virtual ~Blocker() {}
         virtual bool should_unblock(Thread&, time_t now_s, long us) = 0;
         virtual const char* state_string() const = 0;
+        void set_interrupted_by_death() { m_was_interrupted_by_death = true; }
+        bool was_interrupted_by_death() const { return m_was_interrupted_by_death; }
         void set_interrupted_by_signal() { m_was_interrupted_while_blocked = true; }
         bool was_interrupted_by_signal() const { return m_was_interrupted_while_blocked; }
 
     private:
         bool m_was_interrupted_while_blocked { false };
+        bool m_was_interrupted_by_death { false };
         friend class Thread;
     };
 
@@ -260,6 +263,7 @@ public:
     enum class BlockResult {
         WokeNormally,
         InterruptedBySignal,
+        InterruptedByDeath,
     };
 
     template<typename T, class... Args>
@@ -285,6 +289,9 @@ public:
         if (t.was_interrupted_by_signal())
             return BlockResult::InterruptedBySignal;
 
+        if (t.was_interrupted_by_death())
+            return BlockResult::InterruptedByDeath;
+
         return BlockResult::WokeNormally;
     }
 
diff --git a/Tests/Kernel/null-deref-crash-during-pthread_join.cpp b/Tests/Kernel/null-deref-crash-during-pthread_join.cpp
new file mode 100644
index 0000000000..409cbe90f3
--- /dev/null
+++ b/Tests/Kernel/null-deref-crash-during-pthread_join.cpp
@@ -0,0 +1,21 @@
+#include <pthread.h>
+#include <stdio.h>
+#include <sys/select.h>
+#include <unistd.h>
+
+int main(int, char**)
+{
+    pthread_t tid;
+    pthread_create(
+        &tid, nullptr, [](void*) -> void* {
+            sleep(1);
+            asm volatile("ud2");
+            return nullptr;
+        },
+        nullptr);
+
+    pthread_join(tid, nullptr);
+
+    printf("ok\n");
+    return 0;
+}