From b4e478aa508407b999766efea882e4ec4f2f3b6d Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 30 Jan 2019 18:26:19 +0100 Subject: [PATCH] Deallocate PTY's when they close. This required a fair bit of plumbing. The CharacterDevice::close() virtual will now be closed by ~FileDescriptor(), allowing device implementations to do custom cleanup at that point. One big problem remains: if the master PTY is closed before the slave PTY, we go into crashy land. --- AK/Badge.h | 7 +++++++ Kernel/CharacterDevice.cpp | 4 ++++ Kernel/CharacterDevice.h | 1 + Kernel/DevPtsFS.cpp | 3 ++- Kernel/DevPtsFS.h | 2 +- Kernel/FileDescriptor.cpp | 9 ++++++++- Kernel/FileDescriptor.h | 2 ++ Kernel/MasterPTY.cpp | 22 ++++++++++++++++++++-- Kernel/MasterPTY.h | 6 ++++-- Kernel/PTYMultiplexer.cpp | 20 ++++++++++++++++++++ Kernel/PTYMultiplexer.h | 6 ++++++ Kernel/Process.cpp | 12 +++++++++--- Kernel/Process.h | 3 ++- Kernel/SlavePTY.cpp | 6 ++++++ Kernel/SlavePTY.h | 1 + Kernel/VirtualFileSystem.cpp | 5 +++++ Kernel/VirtualFileSystem.h | 1 + Kernel/init.cpp | 1 + Terminal/main.cpp | 5 ++++- 19 files changed, 104 insertions(+), 12 deletions(-) create mode 100644 AK/Badge.h diff --git a/AK/Badge.h b/AK/Badge.h new file mode 100644 index 00000000000..46ae8fbce98 --- /dev/null +++ b/AK/Badge.h @@ -0,0 +1,7 @@ +#pragma once + +template +class Badge { + friend T; + Badge() { } +}; diff --git a/Kernel/CharacterDevice.cpp b/Kernel/CharacterDevice.cpp index b76e6caba42..77ab998dd99 100644 --- a/Kernel/CharacterDevice.cpp +++ b/Kernel/CharacterDevice.cpp @@ -10,6 +10,10 @@ RetainPtr CharacterDevice::open(int& error, int options) return VFS::the().open(*this, error, options); } +void CharacterDevice::close() +{ +} + int CharacterDevice::ioctl(Process&, unsigned, unsigned) { return -ENOTTY; diff --git a/Kernel/CharacterDevice.h b/Kernel/CharacterDevice.h index c37cbf0600c..179a739e1df 100644 --- a/Kernel/CharacterDevice.h +++ b/Kernel/CharacterDevice.h @@ -14,6 +14,7 @@ public: InodeMetadata metadata() const { return { }; } virtual RetainPtr open(int& error, int options); + virtual void close(); virtual bool can_read(Process&) const = 0; virtual bool can_write(Process&) const = 0; diff --git a/Kernel/DevPtsFS.cpp b/Kernel/DevPtsFS.cpp index a59e31b6492..4deaac06e70 100644 --- a/Kernel/DevPtsFS.cpp +++ b/Kernel/DevPtsFS.cpp @@ -1,4 +1,5 @@ -#include "DevPtsFS.h" +#include +#include #include #include diff --git a/Kernel/DevPtsFS.h b/Kernel/DevPtsFS.h index 6c98e6e6395..94587e00311 100644 --- a/Kernel/DevPtsFS.h +++ b/Kernel/DevPtsFS.h @@ -1,10 +1,10 @@ #pragma once -#include "SlavePTY.h" #include #include class Process; +class SlavePTY; class DevPtsFS final : public SynthFS { public: diff --git a/Kernel/FileDescriptor.cpp b/Kernel/FileDescriptor.cpp index 527eda23a79..d2b1fffcb48 100644 --- a/Kernel/FileDescriptor.cpp +++ b/Kernel/FileDescriptor.cpp @@ -40,8 +40,15 @@ FileDescriptor::FileDescriptor(RetainPtr&& device) FileDescriptor::~FileDescriptor() { - if (m_fifo) + if (m_device) { + m_device->close(); + m_device = nullptr; + } + if (m_fifo) { m_fifo->close(fifo_direction()); + m_fifo = nullptr; + } + m_inode = nullptr; } RetainPtr FileDescriptor::clone() diff --git a/Kernel/FileDescriptor.h b/Kernel/FileDescriptor.h index 77477d2423d..38195d4c1ea 100644 --- a/Kernel/FileDescriptor.h +++ b/Kernel/FileDescriptor.h @@ -86,5 +86,7 @@ private: RetainPtr m_fifo; FIFO::Direction m_fifo_direction { FIFO::Neither }; + + bool m_closed { false }; }; diff --git a/Kernel/MasterPTY.cpp b/Kernel/MasterPTY.cpp index 5a688072c47..2f6a90896ab 100644 --- a/Kernel/MasterPTY.cpp +++ b/Kernel/MasterPTY.cpp @@ -1,15 +1,18 @@ #include "MasterPTY.h" #include "SlavePTY.h" +#include "PTYMultiplexer.h" +#include MasterPTY::MasterPTY(unsigned index) : CharacterDevice(10, index) - , m_slave(*new SlavePTY(*this, index)) + , m_slave(adopt(*new SlavePTY(*this, index))) , m_index(index) { } MasterPTY::~MasterPTY() { + PTYMultiplexer::the().notify_master_destroyed(Badge(), m_index); } String MasterPTY::pts_name() const @@ -19,17 +22,23 @@ String MasterPTY::pts_name() const ssize_t MasterPTY::read(Process&, byte* buffer, size_t size) { + if (!m_slave && m_buffer.is_empty()) + return 0; return m_buffer.read(buffer, size); } ssize_t MasterPTY::write(Process&, const byte* buffer, size_t size) { - m_slave.on_master_write(buffer, size); + if (!m_slave) + return -EIO; + m_slave->on_master_write(buffer, size); return size; } bool MasterPTY::can_read(Process&) const { + if (!m_slave) + return true; return !m_buffer.is_empty(); } @@ -38,6 +47,15 @@ bool MasterPTY::can_write(Process&) const return true; } +void MasterPTY::notify_slave_closed(Badge) +{ + dbgprintf("MasterPTY(%u): slave closed, my retains: %u, slave retains: %u\n", m_index, retain_count(), m_slave->retain_count()); + // +1 retain for my MasterPTY::m_slave + // +1 retain for FileDescriptor::m_device + if (m_slave->retain_count() == 2) + m_slave = nullptr; +} + void MasterPTY::on_slave_write(const byte* data, size_t size) { m_buffer.write(data, size); diff --git a/Kernel/MasterPTY.h b/Kernel/MasterPTY.h index cc375feab68..29d71069277 100644 --- a/Kernel/MasterPTY.h +++ b/Kernel/MasterPTY.h @@ -1,7 +1,8 @@ #pragma once +#include #include -#include "DoubleBuffer.h" +#include class SlavePTY; @@ -21,12 +22,13 @@ public: String pts_name() const; void on_slave_write(const byte*, size_t); bool can_write_from_slave() const; + void notify_slave_closed(Badge); private: // ^CharacterDevice virtual const char* class_name() const override { return "MasterPTY"; } - SlavePTY& m_slave; + RetainPtr m_slave; unsigned m_index; DoubleBuffer m_buffer; }; diff --git a/Kernel/PTYMultiplexer.cpp b/Kernel/PTYMultiplexer.cpp index 95da1b96a71..9d2a13e45b9 100644 --- a/Kernel/PTYMultiplexer.cpp +++ b/Kernel/PTYMultiplexer.cpp @@ -3,10 +3,23 @@ #include static const unsigned s_max_pty_pairs = 8; +static PTYMultiplexer* s_the; + +PTYMultiplexer& PTYMultiplexer::the() +{ + ASSERT(s_the); + return *s_the; +} + +void PTYMultiplexer::initialize_statics() +{ + s_the = nullptr; +} PTYMultiplexer::PTYMultiplexer() : CharacterDevice(5, 2) { + s_the = this; m_freelist.ensure_capacity(s_max_pty_pairs); for (int i = s_max_pty_pairs; i > 0; --i) m_freelist.unchecked_append(i - 1); @@ -28,3 +41,10 @@ RetainPtr PTYMultiplexer::open(int& error, int options) dbgprintf("PTYMultiplexer::open: Vending master %u\n", master->index()); return VFS::the().open(move(master), error, options); } + +void PTYMultiplexer::notify_master_destroyed(Badge, unsigned index) +{ + LOCKER(m_lock); + m_freelist.append(index); + dbgprintf("PTYMultiplexer: %u added to freelist\n", index); +} diff --git a/Kernel/PTYMultiplexer.h b/Kernel/PTYMultiplexer.h index ee338fd135a..c2062c2db74 100644 --- a/Kernel/PTYMultiplexer.h +++ b/Kernel/PTYMultiplexer.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include class MasterPTY; @@ -11,6 +12,9 @@ public: PTYMultiplexer(); virtual ~PTYMultiplexer() override; + static PTYMultiplexer& the(); + static void initialize_statics(); + // ^CharacterDevice virtual RetainPtr open(int& error, int options) override; virtual ssize_t read(Process&, byte*, size_t) override { return 0; } @@ -18,6 +22,8 @@ public: virtual bool can_read(Process&) const override { return true; } virtual bool can_write(Process&) const override { return true; } + void notify_master_destroyed(Badge, unsigned index); + private: // ^CharacterDevice virtual const char* class_name() const override { return "PTYMultiplexer"; } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 61e388cc41f..13f892acab0 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -735,7 +735,7 @@ void Process::sys$exit(int status) kprintf("sys$exit: %s(%u) exit with status %d\n", name().characters(), pid(), status); #endif - set_state(Dead); + die(); m_termination_status = status; m_termination_signal = 0; @@ -750,7 +750,7 @@ void Process::terminate_due_to_signal(byte signal) dbgprintf("terminate_due_to_signal %s(%u) <- %u\n", name().characters(), pid(), signal); m_termination_status = 0; m_termination_signal = signal; - set_state(Dead); + die(); } void Process::send_signal(byte signal, Process* sender) @@ -935,8 +935,8 @@ void Process::crash() ASSERT_INTERRUPTS_DISABLED(); ASSERT(state() != Dead); m_termination_signal = SIGSEGV; - set_state(Dead); dumpRegions(); + die(); Scheduler::pick_next_and_switch_now(); ASSERT_NOT_REACHED(); } @@ -2130,3 +2130,9 @@ int Process::sys$chmod(const char* pathname, mode_t mode) return error; return 0; } + +void Process::die() +{ + set_state(Dead); + m_fds.clear(); +} diff --git a/Kernel/Process.h b/Kernel/Process.h index a92e4effec4..66f6676ce43 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -128,6 +128,7 @@ public: void setSelector(word s) { m_farPtr.selector = s; } void set_state(State s) { m_state = s; } + void die(); pid_t sys$setsid(); pid_t sys$getsid(pid_t); @@ -315,7 +316,7 @@ private: struct FileDescriptorAndFlags { operator bool() const { return !!descriptor; } void clear() { descriptor = nullptr; flags = 0; } - void set(RetainPtr&& d, dword f = 0) { descriptor = move(d), flags = f; } + void set(RetainPtr&& d, dword f = 0) { descriptor = move(d); flags = f; } RetainPtr descriptor; dword flags { 0 }; }; diff --git a/Kernel/SlavePTY.cpp b/Kernel/SlavePTY.cpp index 5e0c66541f5..9944a33a3a1 100644 --- a/Kernel/SlavePTY.cpp +++ b/Kernel/SlavePTY.cpp @@ -15,6 +15,7 @@ SlavePTY::SlavePTY(MasterPTY& master, unsigned index) SlavePTY::~SlavePTY() { DevPtsFS::the().unregister_slave_pty(*this); + VFS::the().unregister_character_device(*this); } String SlavePTY::tty_name() const @@ -37,3 +38,8 @@ bool SlavePTY::can_write(Process&) const { return m_master.can_write_from_slave(); } + +void SlavePTY::close() +{ + m_master.notify_slave_closed(Badge()); +} diff --git a/Kernel/SlavePTY.h b/Kernel/SlavePTY.h index 4d7fcaf1477..a2a95cf051c 100644 --- a/Kernel/SlavePTY.h +++ b/Kernel/SlavePTY.h @@ -22,6 +22,7 @@ private: // ^CharacterDevice virtual bool can_write(Process&) const override; virtual const char* class_name() const override { return "SlavePTY"; } + virtual void close() override; friend class MasterPTY; SlavePTY(MasterPTY&, unsigned index); diff --git a/Kernel/VirtualFileSystem.cpp b/Kernel/VirtualFileSystem.cpp index 5b187f92321..261fdc9493e 100644 --- a/Kernel/VirtualFileSystem.cpp +++ b/Kernel/VirtualFileSystem.cpp @@ -510,6 +510,11 @@ void VFS::register_character_device(CharacterDevice& device) m_character_devices.set(encodedDevice(device.major(), device.minor()), &device); } +void VFS::unregister_character_device(CharacterDevice& device) +{ + m_character_devices.remove(encodedDevice(device.major(), device.minor())); +} + void VFS::for_each_mount(Function callback) const { for (auto& mount : m_mounts) { diff --git a/Kernel/VirtualFileSystem.h b/Kernel/VirtualFileSystem.h index 23d9c37a961..563a2e89e9f 100644 --- a/Kernel/VirtualFileSystem.h +++ b/Kernel/VirtualFileSystem.h @@ -72,6 +72,7 @@ public: bool chmod(const String& path, mode_t, Inode& base, int& error); void register_character_device(CharacterDevice&); + void unregister_character_device(CharacterDevice&); size_t mount_count() const { return m_mounts.size(); } void for_each_mount(Function) const; diff --git a/Kernel/init.cpp b/Kernel/init.cpp index 37d9bdde11d..0847c8c2432 100644 --- a/Kernel/init.cpp +++ b/Kernel/init.cpp @@ -142,6 +142,7 @@ void init() gdt_init(); idt_init(); + PTYMultiplexer::initialize_statics(); VFS::initialize_globals(); vfs = new VFS; diff --git a/Terminal/main.cpp b/Terminal/main.cpp index 34a3b7cd5ab..aabce8b0b1b 100644 --- a/Terminal/main.cpp +++ b/Terminal/main.cpp @@ -97,7 +97,10 @@ int main(int, char**) perror("read(ptm)"); continue; } - assert(nread != 0); + if (nread == 0) { + dbgprintf("Terminal: EOF on master pty, closing.\n"); + break; + } for (ssize_t i = 0; i < nread; ++i) terminal.on_char(buffer[i]); terminal.update();