esp32/network_ppp: Correctly clean up PPP PCB after close.

If PPP is still connected, freeing the PCB will fail and thus instead we
should trigger a disconnect and wait for the lwIP callback to actually
free the PCB.

When PPP is not connected we should check if the freeing failed, warn
the user if so, and only mark the connection as inactive if not.

When all this happens during garbage collection the best case is that
the PPP connection is already dead, which means the callback will be
called immediately and cleanup will happen correctly. The worst case is
that the connection is still alive, thus we are unable to free the PCB
(lwIP won't let us) and it remains referenced in the netif_list, meaning
a use-after-free happens later when lwIP traverses that linked list.

This change does not fully prevent that, but it *does* improve how the
PPP.active(False) method on the ESP32 port behaves: It no longer
immediately tries to free (and fails), but instead triggers a disconnect
and lets the cleanup happen correctly through the status callback.

Signed-off-by: Daniël van de Giessen <daniel@dvdgiessen.nl>

2 files changed, 19 insertions, 18 deletions

diff --git a/extmod/network_ppp_lwip.c b/extmod/network_ppp_lwip.c
index 883275f48..12205521f 100644
--- a/extmod/network_ppp_lwip.c
+++ b/extmod/network_ppp_lwip.c
@@ -119,17 +119,18 @@ static mp_obj_t network_ppp_make_new(const mp_obj_type_t *type, size_t n_args, s
 
 static mp_obj_t network_ppp___del__(mp_obj_t self_in) {
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(self_in);
-    if (self->state >= STATE_ACTIVE) {
-        if (self->state >= STATE_ERROR) {
-            // Still connected over the stream.
-            // Force the connection to close, with nocarrier=1.
-            self->state = STATE_INACTIVE;
-            ppp_close(self->pcb, 1);
-        }
-        network_ppp_stream_uart_irq_disable(self);
+
+    network_ppp_stream_uart_irq_disable(self);
+    if (self->state >= STATE_ERROR) {
+        // Still connected over the stream.
+        // Force the connection to close, with nocarrier=1.
+        ppp_close(self->pcb, 1);
+    } else if (self->state >= STATE_ACTIVE) {
         // Free PPP PCB and reset state.
+        if (ppp_free(self->pcb) != ERR_OK) {
+            mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("ppp_free failed"));
+        }
         self->state = STATE_INACTIVE;
-        ppp_free(self->pcb);
         self->pcb = NULL;
     }
     return mp_const_none;
diff --git a/ports/esp32/network_ppp.c b/ports/esp32/network_ppp.c
index 3a9cb239d..18e0c8816 100644
--- a/ports/esp32/network_ppp.c
+++ b/ports/esp32/network_ppp.c
@@ -128,17 +128,17 @@ static mp_obj_t network_ppp_make_new(const mp_obj_type_t *type, size_t n_args, s
 
 static mp_obj_t network_ppp___del__(mp_obj_t self_in) {
     network_ppp_obj_t *self = MP_OBJ_TO_PTR(self_in);
-    if (self->state >= STATE_ACTIVE) {
-        if (self->state >= STATE_ERROR) {
-            // Still connected over the stream.
-            // Force the connection to close, with nocarrier=1.
-            self->state = STATE_INACTIVE;
-            pppapi_close(self->pcb, 1);
-        }
-        network_ppp_stream_uart_irq_disable(self);
+    network_ppp_stream_uart_irq_disable(self);
+    if (self->state >= STATE_ERROR) {
+        // Still connected over the stream.
+        // Force the connection to close, with nocarrier=1.
+        pppapi_close(self->pcb, 1);
+    } else if (self->state >= STATE_ACTIVE) {
         // Free PPP PCB and reset state.
+        if (pppapi_free(self->pcb) != ERR_OK) {
+            mp_raise_msg(&mp_type_OSError, MP_ERROR_TEXT("pppapi_free failed"));
+        }
         self->state = STATE_INACTIVE;
-        pppapi_free(self->pcb);
         self->pcb = NULL;
     }
     return mp_const_none;