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

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.

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

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

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;

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

Kernel/Thread.h

@@ -99,11 +99,14 @@ class Thread {
  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 @@ class Thread {
  enum class BlockResult {
  WokeNormally,
  InterruptedBySignal,
+ InterruptedByDeath,
  };
 
  template<typename T, class... Args>
@@ -285,6 +289,9 @@ class Thread {
  if (t.was_interrupted_by_signal())
  return BlockResult::InterruptedBySignal;
 
+ if (t.was_interrupted_by_death())
+ return BlockResult::InterruptedByDeath;
+
  return BlockResult::WokeNormally;
  }
 

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