From 11ead5c84fdfb4327270650aa63d964110722bb6 Mon Sep 17 00:00:00 2001 From: Liav A Date: Mon, 4 Mar 2024 21:19:31 +0200 Subject: [PATCH] Kernel: Get RefPtr from the DeviceManagement::get_device method Instead of returning a raw pointer, which could be technically invalid when using it in the caller function, we return a valid RefPtr of such device. This ensures that the code in DevPtsFS is now safe from a rare race condition in which the SlavePTY device is gone but we still have a pointer to it. --- Kernel/Devices/DeviceManagement.cpp | 6 +++--- Kernel/Devices/DeviceManagement.h | 2 +- Kernel/Devices/Storage/StorageManagement.cpp | 4 ++-- Kernel/FileSystem/DevPtsFS/FileSystem.cpp | 10 +++++----- Kernel/FileSystem/DevPtsFS/Inode.cpp | 13 +++++++++---- Kernel/FileSystem/DevPtsFS/Inode.h | 5 ++++- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/Kernel/Devices/DeviceManagement.cpp b/Kernel/Devices/DeviceManagement.cpp index e48c296054..b40892f67a 100644 --- a/Kernel/Devices/DeviceManagement.cpp +++ b/Kernel/Devices/DeviceManagement.cpp @@ -42,13 +42,13 @@ DeviceManagement& DeviceManagement::the() return *s_the; } -Device* DeviceManagement::get_device(MajorNumber major, MinorNumber minor) +RefPtr DeviceManagement::get_device(MajorNumber major, MinorNumber minor) { - return m_devices.with([&](auto& map) -> Device* { + return m_devices.with([&](auto& map) -> RefPtr { auto it = map.find(encoded_device(major.value(), minor.value())); if (it == map.end()) return nullptr; - return it->value; + return *it->value; }); } diff --git a/Kernel/Devices/DeviceManagement.h b/Kernel/Devices/DeviceManagement.h index 180e160c45..ccb5d51828 100644 --- a/Kernel/Devices/DeviceManagement.h +++ b/Kernel/Devices/DeviceManagement.h @@ -41,7 +41,7 @@ public: void for_each(Function); ErrorOr try_for_each(Function(Device&)>); - Device* get_device(MajorNumber major, MinorNumber minor); + RefPtr get_device(MajorNumber major, MinorNumber minor); NullDevice const& null_device() const; NullDevice& null_device(); diff --git a/Kernel/Devices/Storage/StorageManagement.cpp b/Kernel/Devices/Storage/StorageManagement.cpp index 46b2a90c1f..10cfb31358 100644 --- a/Kernel/Devices/Storage/StorageManagement.cpp +++ b/Kernel/Devices/Storage/StorageManagement.cpp @@ -353,9 +353,9 @@ UNMAP_AFTER_INIT void StorageManagement::determine_block_boot_device() // Note: We simply fetch the corresponding BlockDevice with the major and minor parameters. // We don't try to accept and resolve a partition number as it will make this code much more // complicated. This rule is also explained in the boot_device_addressing(7) manual page. - LockRefPtr device = DeviceManagement::the().get_device(parameters_view[0], parameters_view[1]); + auto device = DeviceManagement::the().get_device(parameters_view[0], parameters_view[1]); if (device && device->is_block_device()) - m_boot_block_device = static_ptr_cast(device); + m_boot_block_device = *static_ptr_cast(device); } UNMAP_AFTER_INIT void StorageManagement::determine_boot_device_with_logical_unit_number() diff --git a/Kernel/FileSystem/DevPtsFS/FileSystem.cpp b/Kernel/FileSystem/DevPtsFS/FileSystem.cpp index 027aa15d0d..499b3a3c4c 100644 --- a/Kernel/FileSystem/DevPtsFS/FileSystem.cpp +++ b/Kernel/FileSystem/DevPtsFS/FileSystem.cpp @@ -23,7 +23,7 @@ DevPtsFS::~DevPtsFS() = default; ErrorOr DevPtsFS::initialize() { - m_root_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevPtsFSInode(*this, 1, nullptr))); + m_root_inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevPtsFSInode(*this))); m_root_inode->m_metadata.inode = { fsid(), 1 }; m_root_inode->m_metadata.mode = 0040555; m_root_inode->m_metadata.uid = 0; @@ -55,15 +55,15 @@ ErrorOr> DevPtsFS::get_inode(InodeIdentifier inode_id) cons return *m_root_inode; unsigned pty_index = inode_index_to_pty_index(inode_id.index()); - auto* device = DeviceManagement::the().get_device(201, pty_index); + auto device = DeviceManagement::the().get_device(201, pty_index); VERIFY(device); - auto* pts_device = static_cast(device); + auto& pts_device = static_cast(*device); auto inode = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) DevPtsFSInode(const_cast(*this), inode_id.index(), pts_device))); inode->m_metadata.inode = inode_id; inode->m_metadata.size = 0; - inode->m_metadata.uid = pts_device->uid(); - inode->m_metadata.gid = pts_device->gid(); + inode->m_metadata.uid = pts_device.uid(); + inode->m_metadata.gid = pts_device.gid(); inode->m_metadata.mode = 0020600; inode->m_metadata.major_device = device->major(); inode->m_metadata.minor_device = device->minor(); diff --git a/Kernel/FileSystem/DevPtsFS/Inode.cpp b/Kernel/FileSystem/DevPtsFS/Inode.cpp index 134defca02..291f0f1306 100644 --- a/Kernel/FileSystem/DevPtsFS/Inode.cpp +++ b/Kernel/FileSystem/DevPtsFS/Inode.cpp @@ -16,11 +16,16 @@ static InodeIndex pty_index_to_inode_index(unsigned pty_index) return pty_index + 2; } -DevPtsFSInode::DevPtsFSInode(DevPtsFS& fs, InodeIndex index, SlavePTY* pty) - : Inode(fs, index) +// NOTE: This constructor is used for the root inode only. +DevPtsFSInode::DevPtsFSInode(DevPtsFS& fs) + : Inode(fs, 1) +{ +} + +DevPtsFSInode::DevPtsFSInode(DevPtsFS& fs, InodeIndex index, SlavePTY& pty) + : Inode(fs, index) + , m_pty(pty) { - if (pty) - m_pty = *pty; } DevPtsFSInode::~DevPtsFSInode() = default; diff --git a/Kernel/FileSystem/DevPtsFS/Inode.h b/Kernel/FileSystem/DevPtsFS/Inode.h index 3a581a530c..8a78a6c3ce 100644 --- a/Kernel/FileSystem/DevPtsFS/Inode.h +++ b/Kernel/FileSystem/DevPtsFS/Inode.h @@ -23,7 +23,10 @@ public: DevPtsFS const& fs() const { return static_cast(Inode::fs()); } private: - DevPtsFSInode(DevPtsFS&, InodeIndex, SlavePTY*); + DevPtsFSInode(DevPtsFS&, InodeIndex, SlavePTY&); + + // NOTE: This constructor is used for the root inode only. + DevPtsFSInode(DevPtsFS&); // ^Inode virtual ErrorOr read_bytes_locked(off_t, size_t, UserOrKernelBuffer& buffer, OpenFileDescription*) const override;