Avoid freeing memory in detachInterrupt

user/tests/wiring/no_fixture/interrupts.cpp

@@ -81,6 +81,27 @@ class TestHandler
 	static int count;
 };
 
+class DetachingHandler
+{
+	public:
+		DetachingHandler(pin_t p): pin(p) {}
+
+		void handler()
+		{
+			cnt++;
+			detachInterrupt(pin);
+		}
+
+		int count() {
+			return cnt;
+		}
+
+	private:
+		pin_t pin;
+		DetachingHandler *next_handler;
+		volatile int cnt = 0;
+};
+
 } // namespace
 
 #if !HAL_PLATFORM_NRF52840 // TODO
@@ -95,7 +116,7 @@ test(INTERRUPTS_02_detached_handler_is_destroyed)
 	attachSystemInterrupt(SysInterrupt_SysTick, TestHandler()); // Override current handler
 	assertEqual(TestHandler::count, 1); // Previous handler has been destroyed
 	detachSystemInterrupt(SysInterrupt_SysTick);
-	assertEqual(TestHandler::count, 0);
+	assertEqual(TestHandler::count, 1); // Detaching does not destroy the handler since this would prevent use in an ISR
 }
 
 test(INTERRUPTS_03_isisr_willpreempt_servicedirqn)
@@ -118,6 +139,16 @@ test(INTERRUPTS_03_isisr_willpreempt_servicedirqn)
 #endif // !HAL_PLATFORM_NRF52840
 
 #if PLATFORM_ID == PLATFORM_PHOTON_PRODUCTION || PLATFORM_ID == PLATFORM_P1 || PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION
+void trigger_falling_interrupt(pin_t pin) {
+	auto pinmap = HAL_Pin_Map();
+	static const uint16_t exti_line = pinmap[pin].gpio_pin;
+
+	pinMode(pin, INPUT_PULLDOWN);
+	EXTI_GenerateSWInterrupt(exti_line);
+	pinMode(pin, INPUT_PULLUP);
+}
+
+
 test(INTERRUPTS_04_attachInterruptDirect) {
 	const pin_t pin = D1;
 	const IRQn_Type irqn = EXTI9_5_IRQn;
@@ -132,9 +163,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
 	}, FALLING);
 
 	// Trigger
-	pinMode(pin, INPUT_PULLDOWN);
-	EXTI_GenerateSWInterrupt(exti_line);
-	pinMode(pin, INPUT_PULLUP);
+	trigger_falling_interrupt(pin);
 
 	// attachInterrupt handler should have been called
 	assertTrue(attachInterruptHandler == true);
@@ -148,9 +177,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
 	assertTrue(res);
 
 	// Trigger
-	pinMode(pin, INPUT_PULLDOWN);
-	EXTI_GenerateSWInterrupt(exti_line);
-	pinMode(pin, INPUT_PULLUP);
+	trigger_falling_interrupt(pin);
 
 	// Only a direct handler should have been called
 	assertTrue(attachInterruptDirectHandler == true);
@@ -163,9 +190,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
 	assertTrue(res);
 
 	// Trigger
-	pinMode(pin, INPUT_PULLDOWN);
-	EXTI_GenerateSWInterrupt(exti_line);
-	pinMode(pin, INPUT_PULLUP);
+	trigger_falling_interrupt(pin);
 
 	// attachInterrupt handler should have been called
 	assertTrue(attachInterruptHandler == true);
@@ -181,9 +206,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
 	assertTrue(res);
 
 	// Trigger
-	pinMode(pin, INPUT_PULLDOWN);
-	EXTI_GenerateSWInterrupt(exti_line);
-	pinMode(pin, INPUT_PULLUP);
+	trigger_falling_interrupt(pin);
 
 	// Only a direct handler should have been called
 	assertTrue(attachInterruptDirectHandler == true);
@@ -196,9 +219,7 @@ test(INTERRUPTS_04_attachInterruptDirect) {
 	assertTrue(res);
 
 	// Trigger
-	pinMode(pin, INPUT_PULLDOWN);
-	EXTI_GenerateSWInterrupt(exti_line);
-	pinMode(pin, INPUT_PULLUP);
+	trigger_falling_interrupt(pin);
 
 	// Not handler should have been called as IRQ should be disabled
 	assertTrue(attachInterruptHandler == false);
@@ -210,6 +231,34 @@ test(INTERRUPTS_04_attachInterruptDirect_1) {
 
 	detachInterrupt(pin);
 }
+
+test(INTERRUPTS_04_detachFromISR) {
+	// This test validates that calling detachInterrupt in an ISR is safe when attachInterrupt was called with
+	// an instance and member. This uses the same code path as attachInterrupt with a function pointer,
+	// validating that as well. It also validates that we can replace the handler with a new one.
+
+	const pin_t pin = D1;
+
+	DetachingHandler first_handler(pin);
+	DetachingHandler second_handler(pin);
+
+	pinMode(pin, INPUT_PULLUP);
+	attachInterrupt(pin, &DetachingHandler::handler, &first_handler, FALLING);
+
+	// Trigger
+	trigger_falling_interrupt(pin);
+
+	assertEqual(1, first_handler.count());
+	assertEqual(0, second_handler.count());
+
+	attachInterrupt(pin, &DetachingHandler::handler, &second_handler, FALLING);
+
+	// Trigger
+	trigger_falling_interrupt(pin);
+
+	assertEqual(1, first_handler.count());
+	assertEqual(1, second_handler.count());
+}
 #endif // PLATFORM_ID == PLATFORM_PHOTON_PRODUCTION || PLATFORM_ID == PLATFORM_P1 || PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION
 
 #if PLATFORM_ID == PLATFORM_ELECTRON_PRODUCTION
@@ -220,4 +269,4 @@ test(INTERRUPTS_05_attachInterruptD7) {
 	assertFalse(res);
 	assertFalse(tem);
 }
-#endif
+#endif

wiring/src/spark_wiring_interrupts.cpp

@@ -33,8 +33,12 @@ static wiring_interrupt_handler_t* handlers[TOTAL_PINS];
 
 wiring_interrupt_handler_t* allocate_handler(uint16_t pin, wiring_interrupt_handler_t& fn)
 {
-    delete handlers[pin];
-    return handlers[pin] = new wiring_interrupt_handler_t(fn);
+    if (handlers[pin]) {
+        handlers[pin]->swap(fn);
+    } else {
+        handlers[pin] = new wiring_interrupt_handler_t(fn);
+    }
+    return handlers[pin];
 }
 
 void call_wiring_interrupt_handler(void* data)
@@ -105,10 +109,8 @@ bool detachInterrupt(uint16_t pin)
     if (SYSTEM_ERROR_NONE != HAL_Interrupts_Detach(pin)) {
         return false;
     }
-    if (handlers[pin]) {
-        delete handlers[pin];
-        handlers[pin] = nullptr;
-    }
+    // NB: We do not `delete handlers[pin]` here since this would cause an error
+    // when detachInterrupt is called in an ISR.
     return true;
 }
 
@@ -159,10 +161,9 @@ bool attachSystemInterrupt(hal_irq_t irq, wiring_interrupt_handler_t handler)
  */
 bool detachSystemInterrupt(hal_irq_t irq)
 {
-    HAL_InterruptCallback prev = {};
-    const bool ok = HAL_Set_System_Interrupt_Handler(irq, NULL, &prev, NULL);
-    delete (wiring_interrupt_handler_t*)prev.data;
-    return ok;
+    // NB: We do not delete the previous handler here since this would cause an error
+    // when detachInterrupt is called in an ISR.
+    return HAL_Set_System_Interrupt_Handler(irq, NULL, NULL, NULL);
 }
 
 bool attachInterruptDirect(IRQn_Type irq, HAL_Direct_Interrupt_Handler handler, bool enable)