Kernel+LibC: Clean up how assertions work in the kernel and LibC

This also brings LibC's abort() function closer to the spec.
fleximus · Apr 18, 2021 · f033416 · f033416
1 parent 33a9b2a
commit f033416
Show file tree

Hide file tree

Showing 13 changed files with 36 additions and 73 deletions.
diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h
@@ -189,7 +189,6 @@ namespace Kernel {
  S(prctl) \
  S(mremap) \
  S(set_coredump_metadata) \
- S(abort) \
  S(anon_create) \
  S(msyscall) \
  S(readv) \

diff --git a/Kernel/Arch/i386/CPU.cpp b/Kernel/Arch/i386/CPU.cpp
@@ -33,6 +33,7 @@
 #include <Kernel/Arch/x86/ISRStubs.h>
 #include <Kernel/Arch/x86/ProcessorInfo.h>
 #include <Kernel/Arch/x86/SafeMem.h>
+#include <Kernel/Assertions.h>
 #include <Kernel/Debug.h>
 #include <Kernel/IO.h>
 #include <Kernel/Interrupts/APIC.h>
@@ -2419,6 +2420,13 @@ void __assertion_failed(const char* msg, const char* file, unsigned line, const
  dmesgln("ASSERTION FAILED: {}", msg);
  dmesgln("{}:{} in {}", file, line, func);
 
+ abort();
+}
+#endif
+
+[[noreturn]] void abort()
+{
+#ifdef DEBUG
  // Switch back to the current process's page tables if there are any.
  // Otherwise stack walking will be a disaster.
  auto process = Process::current();
@@ -2427,9 +2435,17 @@ void __assertion_failed(const char* msg, const char* file, unsigned line, const
 
  Kernel::dump_backtrace();
  Processor::halt();
-}
 #endif
 
+ abort();
+}
+
+[[noreturn]] void _abort()
+{
+ asm volatile("ud2");
+ __builtin_unreachable();
+}
+
 NonMaskableInterruptDisabler::NonMaskableInterruptDisabler()
 {
  IO::out8(0x70, IO::in8(0x70) | 0x80);

diff --git a/Kernel/Assertions.h b/Kernel/Assertions.h
@@ -35,12 +35,13 @@
 # define VERIFY_NOT_REACHED() VERIFY(false)
 #else
 # define VERIFY(expr)
-# define VERIFY_NOT_REACHED() CRASH()
+# define VERIFY_NOT_REACHED() _abort()
 #endif
-#define CRASH() \
- do { \
- asm volatile("ud2"); \
- } while (0)
+
+extern "C" {
+[[noreturn]] void _abort();
+[[noreturn]] void abort();
+}
 
 #define VERIFY_INTERRUPTS_DISABLED() VERIFY(!(cpu_flags() & 0x200))
 #define VERIFY_INTERRUPTS_ENABLED() VERIFY(cpu_flags() & 0x200)

diff --git a/Kernel/CMakeLists.txt b/Kernel/CMakeLists.txt
@@ -131,7 +131,6 @@ set(KERNEL_SOURCES
  StdLib.cpp
  Syscall.cpp
  Syscalls/anon_create.cpp
- Syscalls/abort.cpp
  Syscalls/access.cpp
  Syscalls/alarm.cpp
  Syscalls/beep.cpp

diff --git a/Kernel/Process.h b/Kernel/Process.h
@@ -408,7 +408,6 @@ class Process
  KResultOr<FlatPtr> sys$allocate_tls(size_t);
  KResultOr<int> sys$prctl(int option, FlatPtr arg1, FlatPtr arg2);
  KResultOr<int> sys$set_coredump_metadata(Userspace<const Syscall::SC_set_coredump_metadata_params*>);
- [[noreturn]] void sys$abort();
  KResultOr<int> sys$anon_create(size_t, int options);
 
  template<bool sockname, typename Params>

diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp
@@ -99,7 +99,7 @@ KResultOr<FlatPtr> handle(RegisterState& regs, FlatPtr function, FlatPtr arg1, F
  auto& process = current_thread->process();
  current_thread->did_syscall();
 
- if (function == SC_abort || function == SC_exit || function == SC_exit_thread) {
+ if (function == SC_exit || function == SC_exit_thread) {
  // These syscalls need special handling since they never return to the caller.
 
  if (auto* tracer = process.tracer(); tracer && tracer->is_tracing_syscalls()) {
@@ -109,9 +109,6 @@ KResultOr<FlatPtr> handle(RegisterState& regs, FlatPtr function, FlatPtr arg1, F
  }
 
  switch (function) {
- case SC_abort:
- process.sys$abort();
- break;
  case SC_exit:
  process.sys$exit(arg1);
  break;

diff --git a/Kernel/Syscalls/abort.cpp b/Kernel/Syscalls/abort.cpp
diff --git a/Userland/DevTools/UserspaceEmulator/Emulator.h b/Userland/DevTools/UserspaceEmulator/Emulator.h
@@ -163,7 +163,6 @@ class Emulator {
  int virt$connect(int sockfd, FlatPtr address, socklen_t address_size);
  int virt$shutdown(int sockfd, int how);
  void virt$sync();
- void virt$abort();
  void virt$exit(int);
  ssize_t virt$getrandom(FlatPtr buffer, size_t buffer_size, unsigned int flags);
  int virt$chdir(FlatPtr, size_t);

diff --git a/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp b/Userland/DevTools/UserspaceEmulator/Emulator_syscalls.cpp
@@ -220,9 +220,6 @@ u32 Emulator::virt_syscall(u32 function, u32 arg1, u32 arg2, u32 arg3)
  case SC_sync:
  virt$sync();
  return 0;
- case SC_abort:
- virt$abort();
- return 0;
  case SC_exit:
  virt$exit((int)arg1);
  return 0;
@@ -1036,14 +1033,6 @@ void Emulator::virt$sync()
  syscall(SC_sync);
 }
 
-void Emulator::virt$abort()
-{
- reportln("\n=={}== \033[33;1mSyscall: abort\033[0m, shutting down!", getpid());
- m_exit_status = 127;
- m_shutdown = true;
- dump_backtrace();
-}
-
 void Emulator::virt$exit(int status)
 {
  reportln("\n=={}== \033[33;1mSyscall: exit({})\033[0m, shutting down!", getpid(), status);

diff --git a/Userland/Libraries/LibC/assert.cpp b/Userland/Libraries/LibC/assert.cpp
@@ -48,13 +48,12 @@ void __assertion_failed(const char* msg)
  { msg, strlen(msg) },
  };
  syscall(SC_set_coredump_metadata, &params);
- syscall(SC_abort);
- for (;;) { }
+ abort();
 }
 #endif
 }
 
-void __crash()
+void _abort()
 {
  asm volatile("ud2");
  __builtin_unreachable();

diff --git a/Userland/Libraries/LibC/assert.h b/Userland/Libraries/LibC/assert.h
@@ -31,7 +31,7 @@
 __BEGIN_DECLS
 
 #ifdef DEBUG
-__attribute__((noreturn)) void __assertion_failed(const char* msg);
+[[noreturn]] void __assertion_failed(const char* msg);
 # define __stringify_helper(x) # x
 # define __stringify(x) __stringify_helper(x)
 # define assert(expr) \
@@ -42,12 +42,11 @@ __attribute__((noreturn)) void __assertion_failed(const char* msg);
 # define VERIFY_NOT_REACHED() assert(false)
 #else
 # define assert(expr) ((void)(0))
-# define VERIFY_NOT_REACHED() CRASH()
+# define VERIFY_NOT_REACHED() _abort()
 #endif
 
-__attribute__((noreturn)) void __crash();
+[[noreturn]] void _abort();
 
-#define CRASH() __crash()
 #define VERIFY assert
 #define TODO VERIFY_NOT_REACHED
 

diff --git a/Userland/Libraries/LibC/stdlib.cpp b/Userland/Libraries/LibC/stdlib.cpp
@@ -249,8 +249,12 @@ void abort()
  // For starters, send ourselves a SIGABRT.
  raise(SIGABRT);
  // If that didn't kill us, try harder.
- raise(SIGKILL);
- _exit(127);
+ sigset_t set;
+ sigemptyset(&set);
+ sigaddset(&set, SIGABRT);
+ sigprocmask(SIG_UNBLOCK, &set, nullptr);
+ raise(SIGABRT);
+ _abort();
 }
 
 static HashTable<const char*> s_malloced_environment_variables;

diff --git a/Userland/Tests/Kernel/fuzz-syscalls.cpp b/Userland/Tests/Kernel/fuzz-syscalls.cpp
@@ -38,7 +38,7 @@
 
 static bool is_deadly_syscall(int fn)
 {
- return fn == SC_exit || fn == SC_fork || fn == SC_sigreturn || fn == SC_exit_thread || fn == SC_abort;
+ return fn == SC_exit || fn == SC_fork || fn == SC_sigreturn || fn == SC_exit_thread;
 }
 
 static bool is_unfuzzable_syscall(int fn)