From f033416893b097e9f148711c161537db328d02cd Mon Sep 17 00:00:00 2001 From: Gunnar Beutner Date: Sun, 18 Apr 2021 08:43:10 +0200 Subject: [PATCH] Kernel+LibC: Clean up how assertions work in the kernel and LibC This also brings LibC's abort() function closer to the spec. --- Kernel/API/Syscall.h | 1 - Kernel/Arch/i386/CPU.cpp | 18 ++++++++- Kernel/Assertions.h | 11 +++--- Kernel/CMakeLists.txt | 1 - Kernel/Process.h | 1 - Kernel/Syscall.cpp | 5 +-- Kernel/Syscalls/abort.cpp | 38 ------------------- .../DevTools/UserspaceEmulator/Emulator.h | 1 - .../UserspaceEmulator/Emulator_syscalls.cpp | 11 ------ Userland/Libraries/LibC/assert.cpp | 5 +-- Userland/Libraries/LibC/assert.h | 7 ++-- Userland/Libraries/LibC/stdlib.cpp | 8 +++- Userland/Tests/Kernel/fuzz-syscalls.cpp | 2 +- 13 files changed, 36 insertions(+), 73 deletions(-) delete mode 100644 Kernel/Syscalls/abort.cpp diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h index 88aa5a1313..11d6807302 100644 --- 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 index 7217caba51..93c22c1988 100644 --- a/Kernel/Arch/i386/CPU.cpp +++ b/Kernel/Arch/i386/CPU.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -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 index f75860aadb..2fb7bae36b 100644 --- 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 index 7617f32a08..8b376c9749 100644 --- 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 index 043d145eb3..8a4f4f7653 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -408,7 +408,6 @@ public: KResultOr sys$allocate_tls(size_t); KResultOr sys$prctl(int option, FlatPtr arg1, FlatPtr arg2); KResultOr sys$set_coredump_metadata(Userspace); - [[noreturn]] void sys$abort(); KResultOr sys$anon_create(size_t, int options); template diff --git a/Kernel/Syscall.cpp b/Kernel/Syscall.cpp index fc15c2f677..bd7a46bcc0 100644 --- a/Kernel/Syscall.cpp +++ b/Kernel/Syscall.cpp @@ -99,7 +99,7 @@ KResultOr 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 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 deleted file mode 100644 index 7276ac8cda..0000000000 --- a/Kernel/Syscalls/abort.cpp +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright (c) 2021, Andreas Kling - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are met: - * - * 1. Redistributions of source code must retain the above copyright notice, this - * list of conditions and the following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above copyright notice, - * this list of conditions and the following disclaimer in the documentation - * and/or other materials provided with the distribution. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE - * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR - * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER - * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, - * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ - -#include -#include -#include - -namespace Kernel { - -void Process::sys$abort() -{ - crash(SIGABRT, 0); -} - -} diff --git a/Userland/DevTools/UserspaceEmulator/Emulator.h b/Userland/DevTools/UserspaceEmulator/Emulator.h index 35ad2ee7e5..0a1e1c2e6c 100644 --- a/Userland/DevTools/UserspaceEmulator/Emulator.h +++ b/Userland/DevTools/UserspaceEmulator/Emulator.h @@ -163,7 +163,6 @@ private: 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 index 31fa83dae7..3d7d5cad75 100644 --- 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 index 2915d43962..58bcf55c2d 100644 --- 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, ¶ms); - 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 index 3749e66c43..19a28707fa 100644 --- 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 index 3ebc5640ee..202352d003 100644 --- 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 s_malloced_environment_variables; diff --git a/Userland/Tests/Kernel/fuzz-syscalls.cpp b/Userland/Tests/Kernel/fuzz-syscalls.cpp index e2c15b4fb7..15ac24380b 100644 --- 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)