Kernel: Fix GenericInterruptHandler problems with virtual functions

Because registering and unregistering interrupt handlers triggers
calls to virtual functions, we can't do this in the constructor
and destructor.

Fixes #5539

Kernel/Arch/i386/CPU.cpp

@@ -384,64 +384,77 @@ static void unimp_trap()
 
 GenericInterruptHandler& get_interrupt_handler(u8 interrupt_number)
 {
-    VERIFY(s_interrupt_handler[interrupt_number] != nullptr);
-    return *s_interrupt_handler[interrupt_number];
+    auto*& handler_slot = s_interrupt_handler[interrupt_number];
+    VERIFY(handler_slot != nullptr);
+    return *handler_slot;
 }
 
 static void revert_to_unused_handler(u8 interrupt_number)
 {
-    new UnhandledInterruptHandler(interrupt_number);
+    auto handler = new UnhandledInterruptHandler(interrupt_number);
+    handler->register_interrupt_handler();
 }
 
 void register_generic_interrupt_handler(u8 interrupt_number, GenericInterruptHandler& handler)
 {
     VERIFY(interrupt_number < GENERIC_INTERRUPT_HANDLERS_COUNT);
-    if (s_interrupt_handler[interrupt_number] != nullptr) {
-        if (s_interrupt_handler[interrupt_number]->type() == HandlerType::UnhandledInterruptHandler) {
-            s_interrupt_handler[interrupt_number] = &handler;
+    auto*& handler_slot = s_interrupt_handler[interrupt_number];
+    if (handler_slot != nullptr) {
+        if (handler_slot->type() == HandlerType::UnhandledInterruptHandler) {
+            if (handler_slot) {
+                auto* unhandled_handler = static_cast<UnhandledInterruptHandler*>(handler_slot);
+                unhandled_handler->unregister_interrupt_handler();
+                delete unhandled_handler;
+            }
+            handler_slot = &handler;
             return;
         }
-        if (s_interrupt_handler[interrupt_number]->is_shared_handler() && !s_interrupt_handler[interrupt_number]->is_sharing_with_others()) {
-            VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::SharedIRQHandler);
-            static_cast<SharedIRQHandler*>(s_interrupt_handler[interrupt_number])->register_handler(handler);
+        if (handler_slot->is_shared_handler() && !handler_slot->is_sharing_with_others()) {
+            VERIFY(handler_slot->type() == HandlerType::SharedIRQHandler);
+            static_cast<SharedIRQHandler*>(handler_slot)->register_handler(handler);
             return;
         }
-        if (!s_interrupt_handler[interrupt_number]->is_shared_handler()) {
-            if (s_interrupt_handler[interrupt_number]->type() == HandlerType::SpuriousInterruptHandler) {
-                static_cast<SpuriousInterruptHandler*>(s_interrupt_handler[interrupt_number])->register_handler(handler);
+        if (!handler_slot->is_shared_handler()) {
+            if (handler_slot->type() == HandlerType::SpuriousInterruptHandler) {
+                static_cast<SpuriousInterruptHandler*>(handler_slot)->register_handler(handler);
                 return;
             }
-            VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::IRQHandler);
-            auto& previous_handler = *s_interrupt_handler[interrupt_number];
-            s_interrupt_handler[interrupt_number] = nullptr;
+            VERIFY(handler_slot->type() == HandlerType::IRQHandler);
+            auto& previous_handler = *handler_slot;
+            handler_slot = nullptr;
             SharedIRQHandler::initialize(interrupt_number);
-            static_cast<SharedIRQHandler*>(s_interrupt_handler[interrupt_number])->register_handler(previous_handler);
-            static_cast<SharedIRQHandler*>(s_interrupt_handler[interrupt_number])->register_handler(handler);
+            VERIFY(handler_slot);
+            static_cast<SharedIRQHandler*>(handler_slot)->register_handler(previous_handler);
+            static_cast<SharedIRQHandler*>(handler_slot)->register_handler(handler);
             return;
         }
         VERIFY_NOT_REACHED();
     } else {
-        s_interrupt_handler[interrupt_number] = &handler;
+        handler_slot = &handler;
     }
 }
 
 void unregister_generic_interrupt_handler(u8 interrupt_number, GenericInterruptHandler& handler)
 {
-    VERIFY(s_interrupt_handler[interrupt_number] != nullptr);
-    if (s_interrupt_handler[interrupt_number]->type() == HandlerType::UnhandledInterruptHandler) {
+    auto*& handler_slot = s_interrupt_handler[interrupt_number];
+    VERIFY(handler_slot != nullptr);
+    if (handler_slot->type() == HandlerType::UnhandledInterruptHandler) {
         dbgln("Trying to unregister unused handler (?)");
         return;
     }
-    if (s_interrupt_handler[interrupt_number]->is_shared_handler() && !s_interrupt_handler[interrupt_number]->is_sharing_with_others()) {
-        VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::SharedIRQHandler);
-        static_cast<SharedIRQHandler*>(s_interrupt_handler[interrupt_number])->unregister_handler(handler);
-        if (!static_cast<SharedIRQHandler*>(s_interrupt_handler[interrupt_number])->sharing_devices_count()) {
+    if (handler_slot->is_shared_handler() && !handler_slot->is_sharing_with_others()) {
+        VERIFY(handler_slot->type() == HandlerType::SharedIRQHandler);
+        auto* shared_handler = static_cast<SharedIRQHandler*>(handler_slot);
+        shared_handler->unregister_handler(handler);
+        if (!shared_handler->sharing_devices_count()) {
+            handler_slot = nullptr;
             revert_to_unused_handler(interrupt_number);
         }
         return;
     }
-    if (!s_interrupt_handler[interrupt_number]->is_shared_handler()) {
-        VERIFY(s_interrupt_handler[interrupt_number]->type() == HandlerType::IRQHandler);
+    if (!handler_slot->is_shared_handler()) {
+        VERIFY(handler_slot->type() == HandlerType::IRQHandler);
+        handler_slot = nullptr;
         revert_to_unused_handler(interrupt_number);
         return;
     }
@@ -671,7 +684,8 @@ UNMAP_AFTER_INIT static void idt_init()
     dbgln("Installing Unhandled Handlers");
 
     for (u8 i = 0; i < GENERIC_INTERRUPT_HANDLERS_COUNT; ++i) {
-        new UnhandledInterruptHandler(i);
+        auto* handler = new UnhandledInterruptHandler(i);
+        handler->register_interrupt_handler();
     }
 
     flush_idt();

Kernel/Interrupts/APIC.cpp

@@ -87,7 +87,8 @@ public:
 
     static void initialize(u8 interrupt_number)
     {
-        new APICIPIInterruptHandler(interrupt_number);
+        auto* handler = new APICIPIInterruptHandler(interrupt_number);
+        handler->register_interrupt_handler();
     }
 
     virtual void handle_interrupt(const RegisterState&) override;
@@ -117,7 +118,8 @@ public:
 
     static void initialize(u8 interrupt_number)
     {
-        new APICErrInterruptHandler(interrupt_number);
+        auto* handler = new APICErrInterruptHandler(interrupt_number);
+        handler->register_interrupt_handler();
     }
 
     virtual void handle_interrupt(const RegisterState&) override;

Kernel/Interrupts/GenericInterruptHandler.cpp

@@ -40,25 +40,50 @@ GenericInterruptHandler::GenericInterruptHandler(u8 interrupt_number, bool disab
     : m_interrupt_number(interrupt_number)
     , m_disable_remap(disable_remap)
 {
+    // NOTE: We cannot register or unregister the handler while the object
+    // is being constructed or deconstructed!
+}
+
+void GenericInterruptHandler::will_be_destroyed()
+{
+    // This will be called for RefCounted interrupt handlers before the
+    // object is being destroyed. As soon as the destructor is invoked
+    // it is no longer advisable to unregister the handler (which causes
+    // calls to virtual functions), so let's do this right before
+    // invoking it
+    unregister_interrupt_handler();
+}
+
+void GenericInterruptHandler::register_interrupt_handler()
+{
+    if (m_registered)
+        return;
     if (m_disable_remap)
         register_generic_interrupt_handler(m_interrupt_number, *this);
     else
         register_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(m_interrupt_number), *this);
+    m_registered = true;
 }
 
-GenericInterruptHandler::~GenericInterruptHandler()
+void GenericInterruptHandler::unregister_interrupt_handler()
 {
+    if (!m_registered)
+        return;
     if (m_disable_remap)
         unregister_generic_interrupt_handler(m_interrupt_number, *this);
     else
         unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(m_interrupt_number), *this);
+    m_registered = false;
 }
 
 void GenericInterruptHandler::change_interrupt_number(u8 number)
 {
     VERIFY_INTERRUPTS_DISABLED();
     VERIFY(!m_disable_remap);
-    unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this);
+    if (m_registered) {
+        unregister_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this);
+        m_registered = false;
+    }
     m_interrupt_number = number;
     register_generic_interrupt_handler(InterruptManagement::acquire_mapped_interrupt_number(interrupt_number()), *this);
 }

Kernel/Interrupts/GenericInterruptHandler.h

@@ -43,9 +43,17 @@ enum class HandlerType : u8 {
 class GenericInterruptHandler {
 public:
     static GenericInterruptHandler& from(u8 interrupt_number);
-    virtual ~GenericInterruptHandler();
+    virtual ~GenericInterruptHandler()
+    {
+        VERIFY(!m_registered);
+    }
     virtual void handle_interrupt(const RegisterState& regs) = 0;
 
+    void will_be_destroyed();
+    bool is_registered() const { return m_registered; }
+    void register_interrupt_handler();
+    void unregister_interrupt_handler();
+
     u8 interrupt_number() const { return m_interrupt_number; }
 
     size_t get_invoking_count() const { return m_invoking_count; }
@@ -74,5 +82,6 @@ private:
     Atomic<u32, AK::MemoryOrder::memory_order_relaxed> m_invoking_count { 0 };
     u8 m_interrupt_number { 0 };
     bool m_disable_remap { false };
+    bool m_registered { false };
 };
 }

Kernel/Interrupts/IRQHandler.cpp

@@ -56,6 +56,8 @@ bool IRQHandler::eoi()
 void IRQHandler::enable_irq()
 {
     dbgln_if(IRQ_DEBUG, "Enable IRQ {}", interrupt_number());
+    if (!is_registered())
+        register_interrupt_handler();
     m_enabled = true;
     if (!m_shared_with_others)
         m_responsible_irq_controller->enable(*this);

Kernel/Interrupts/SharedIRQHandler.cpp

@@ -36,7 +36,9 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT void SharedIRQHandler::initialize(u8 interrupt_number)
 {
-    new SharedIRQHandler(interrupt_number);
+    auto* handler = new SharedIRQHandler(interrupt_number);
+    handler->register_interrupt_handler();
+    handler->disable_interrupt_vector();
 }
 
 void SharedIRQHandler::register_handler(GenericInterruptHandler& handler)
@@ -71,7 +73,6 @@ SharedIRQHandler::SharedIRQHandler(u8 irq)
 #if INTERRUPT_DEBUG
     klog() << "Shared Interrupt Handler registered @ " << interrupt_number();
 #endif
-    disable_interrupt_vector();
 }
 
 SharedIRQHandler::~SharedIRQHandler()

Kernel/Interrupts/SpuriousInterruptHandler.cpp

@@ -31,7 +31,8 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT void SpuriousInterruptHandler::initialize(u8 interrupt_number)
 {
-    new SpuriousInterruptHandler(interrupt_number);
+    auto* handler = new SpuriousInterruptHandler(interrupt_number);
+    handler->register_interrupt_handler();
 }
 
 void SpuriousInterruptHandler::register_handler(GenericInterruptHandler& handler)

Kernel/Time/APICTimer.cpp

@@ -38,12 +38,12 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT APICTimer* APICTimer::initialize(u8 interrupt_number, HardwareTimerBase& calibration_source)
 {
-    auto* timer = new APICTimer(interrupt_number, nullptr);
+    auto timer = adopt(*new APICTimer(interrupt_number, nullptr));
+    timer->register_interrupt_handler();
     if (!timer->calibrate(calibration_source)) {
-        delete timer;
         return nullptr;
     }
-    return timer;
+    return &timer.leak_ref();
 }
 
 UNMAP_AFTER_INIT APICTimer::APICTimer(u8 interrupt_number, Function<void(const RegisterState&)> callback)

Kernel/Time/APICTimer.h

@@ -51,6 +51,7 @@ public:
     virtual bool is_capable_of_frequency(size_t frequency) const override;
     virtual size_t calculate_nearest_possible_frequency(size_t frequency) const override;
 
+    void will_be_destroyed() { HardwareTimer<GenericInterruptHandler>::will_be_destroyed(); }
     void enable_local_timer();
     void disable_local_timer();
 

Kernel/Time/HPETComparator.cpp

@@ -33,7 +33,9 @@ namespace Kernel {
 
 UNMAP_AFTER_INIT NonnullRefPtr<HPETComparator> HPETComparator::create(u8 number, u8 irq, bool periodic_capable)
 {
-    return adopt(*new HPETComparator(number, irq, periodic_capable));
+    auto timer = adopt(*new HPETComparator(number, irq, periodic_capable));
+    timer->register_interrupt_handler();
+    return timer;
 }
 
 UNMAP_AFTER_INIT HPETComparator::HPETComparator(u8 number, u8 irq, bool periodic_capable)

Kernel/Time/HardwareTimer.h

@@ -49,6 +49,12 @@ class HardwareTimerBase
 public:
     virtual ~HardwareTimerBase() { }
 
+    // We need to create a virtual will_be_destroyed here because we derive
+    // from RefCounted<HardwareTimerBase> here, which means that RefCounted<>
+    // will only call will_be_destroyed if we define it here. The derived
+    // classes then should forward this to e.g. GenericInterruptHandler.
+    virtual void will_be_destroyed() = 0;
+
     virtual const char* model() const = 0;
     virtual HardwareTimerType timer_type() const = 0;
     virtual Function<void(const RegisterState&)> set_callback(Function<void(const RegisterState&)>) = 0;
@@ -73,6 +79,11 @@ class HardwareTimer<IRQHandler>
     : public HardwareTimerBase
     , public IRQHandler {
 public:
+    virtual void will_be_destroyed() override
+    {
+        IRQHandler::will_be_destroyed();
+    }
+
     virtual const char* purpose() const override
     {
         if (TimeManagement::the().is_system_timer(*this))
@@ -115,6 +126,11 @@ class HardwareTimer<GenericInterruptHandler>
     : public HardwareTimerBase
     , public GenericInterruptHandler {
 public:
+    virtual void will_be_destroyed() override
+    {
+        GenericInterruptHandler::will_be_destroyed();
+    }
+
     virtual const char* purpose() const override
     {
         return model();