From 65659a4cf5d107f68fd2c07029afc818191d149c Mon Sep 17 00:00:00 2001 From: Lucas CHOLLET Date: Thu, 27 Nov 2025 14:02:33 +0100 Subject: [PATCH 1/2] Kernel: Only allow absolute paths when setting coredump directory --- .../Kernel/Configuration/CoredumpDirectory.cpp | 7 ++++++- .../Kernel/Configuration/CoredumpDirectory.h | 2 +- .../Kernel/Configuration/StringVariable.cpp | 2 +- .../Kernel/Configuration/StringVariable.h | 2 +- Tests/Kernel/TestProcFSWrite.cpp | 13 +++++++++++++ 5 files changed, 22 insertions(+), 4 deletions(-) diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.cpp index 0e84049eccda7e..8aaa38e8bd8836 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.cpp +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.cpp @@ -28,11 +28,16 @@ ErrorOr> SysFSCoredumpDirectory::value() const return KString::try_create(""sv); }); } -void SysFSCoredumpDirectory::set_value(NonnullOwnPtr new_value) + +ErrorOr SysFSCoredumpDirectory::set_value(NonnullOwnPtr new_value) { + if (new_value->length() > 0 && new_value->bytes()[0] != '/') + return Error::from_errno(EINVAL); + Coredump::directory_path().with([&](auto& coredump_directory_path) { coredump_directory_path = move(new_value); }); + return {}; } mode_t SysFSCoredumpDirectory::permissions() const diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.h b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.h index fee5f43f10e363..e6e3a9b16a4321 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.h +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/CoredumpDirectory.h @@ -20,7 +20,7 @@ class SysFSCoredumpDirectory final : public SysFSSystemStringVariable { private: virtual ErrorOr> value() const override; - virtual void set_value(NonnullOwnPtr new_value) override; + virtual ErrorOr set_value(NonnullOwnPtr new_value) override; explicit SysFSCoredumpDirectory(SysFSDirectory const&); diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.cpp index 5db643398b46e3..eb4a3621adb9aa 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.cpp +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.cpp @@ -28,7 +28,7 @@ ErrorOr SysFSSystemStringVariable::write_bytes(off_t, size_t count, User // NOTE: If we are in a jail, don't let the current process to change the variable. if (Process::current().is_jailed()) return Error::from_errno(EPERM); - set_value(move(new_value_without_possible_newlines)); + TRY(set_value(move(new_value_without_possible_newlines))); return count; } diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.h b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.h index d02a2accba5dcb..da836a22cbb8fa 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.h +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Configuration/StringVariable.h @@ -31,7 +31,7 @@ class SysFSSystemStringVariable : public SysFSGlobalInformation { { } virtual ErrorOr> value() const = 0; - virtual void set_value(NonnullOwnPtr new_value) = 0; + virtual ErrorOr set_value(NonnullOwnPtr new_value) = 0; private: // ^SysFSGlobalInformation diff --git a/Tests/Kernel/TestProcFSWrite.cpp b/Tests/Kernel/TestProcFSWrite.cpp index 0245e99138f5c3..64085231276f71 100644 --- a/Tests/Kernel/TestProcFSWrite.cpp +++ b/Tests/Kernel/TestProcFSWrite.cpp @@ -41,3 +41,16 @@ TEST_CASE(root_writes_to_procfs) FAIL("Wrote successfully?!"); } } + +TEST_CASE(set_coredump_path) +{ + auto fd = open("/sys/kernel/conf/coredump_directory", O_RDWR); + if (fd < 0) { + perror("open"); + FAIL("open failed?! See debugout"); + return; + } + static constexpr auto path = "relative/path"sv; + write(fd, path.characters_without_null_termination(), path.length()); + EXPECT_EQ(errno, EINVAL); +} From 99312a45f568995fea441602d1119683bc683163 Mon Sep 17 00:00:00 2001 From: Lucas CHOLLET Date: Tue, 11 Nov 2025 17:01:12 +0100 Subject: [PATCH 2/2] Kernel+CrashDaemon: Use a ".partial" suffix for incomplete coredumps The Kernel will now write to a .partial file until it is done writing, and then rename the file by removing the extension. This makes the final coredump appear atomically on the file system and thus remove the need for special handling of partial files in CrashDaemon. --- Kernel/Tasks/Process.cpp | 13 +++++++++-- Userland/Services/CrashDaemon/main.cpp | 32 +++++--------------------- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/Kernel/Tasks/Process.cpp b/Kernel/Tasks/Process.cpp index 84119dea9edbe6..b1aed80ab87f15 100644 --- a/Kernel/Tasks/Process.cpp +++ b/Kernel/Tasks/Process.cpp @@ -831,10 +831,19 @@ ErrorOr Process::dump_core() return {}; } auto coredump_path = TRY(name().with([&](auto& process_name) { - return KString::formatted("{}/{}_{}_{}", coredump_directory_path->view(), process_name.representable_view(), pid().value(), kgettimeofday().seconds_since_epoch()); + return KString::formatted("{}/{}_{}_{}.partial", coredump_directory_path->view(), process_name.representable_view(), pid().value(), kgettimeofday().seconds_since_epoch()); })); auto coredump = TRY(Coredump::try_create(*this, coredump_path->view())); - return coredump->write(); + TRY(coredump->write()); + + auto root_custody = vfs_root_context()->root_custody().with([](auto& custody) -> NonnullRefPtr { + return custody; + }); + + auto new_path = TRY(KString::try_create(coredump_path->view().trim(".partial"sv))); + TRY(VirtualFileSystem::rename(vfs_root_context(), credentials(), *root_custody, coredump_path->view(), *root_custody, new_path->view())); + + return {}; } ErrorOr Process::dump_perfcore() diff --git a/Userland/Services/CrashDaemon/main.cpp b/Userland/Services/CrashDaemon/main.cpp index ae8dbbaa290b44..e16165dfd46652 100644 --- a/Userland/Services/CrashDaemon/main.cpp +++ b/Userland/Services/CrashDaemon/main.cpp @@ -4,32 +4,11 @@ * SPDX-License-Identifier: BSD-2-Clause */ -#include -#include #include #include #include #include #include -#include -#include -#include -#include - -static void wait_until_coredump_is_ready(ByteString const& coredump_path) -{ - while (true) { - struct stat statbuf; - if (stat(coredump_path.characters(), &statbuf) < 0) { - perror("stat"); - VERIFY_NOT_REACHED(); - } - if (statbuf.st_mode & 0400) // Check if readable - break; - - usleep(10000); // sleep for 10ms - } -} static void launch_crash_reporter(ByteString const& coredump_path, bool unlink_on_exit) { @@ -49,13 +28,14 @@ ErrorOr serenity_main(Main::Arguments) TRY(watcher.add_watch("/tmp/coredump", Core::FileWatcherEvent::Type::ChildCreated)); while (true) { - auto event = watcher.wait_for_event(); - VERIFY(event.has_value()); - if (event.value().type != Core::FileWatcherEvent::Type::ChildCreated) + auto event = watcher.wait_for_event().value(); + if (event.type != Core::FileWatcherEvent::Type::ChildCreated) continue; - auto& coredump_path = event.value().event_path; + auto& coredump_path = event.event_path; + if (coredump_path.ends_with(".partial"sv)) + continue; + dbgln("New coredump file: {}", coredump_path); - wait_until_coredump_is_ready(coredump_path); auto file_or_error = Core::MappedFile::map(coredump_path); if (file_or_error.is_error()) {