From 8c47ff7153a7a54eced6c8177698339abe4973ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dani=C3=ABl=20van=20de=20Giessen?= Date: Tue, 8 Jul 2025 15:52:17 +0200 Subject: [PATCH] esp32/network_ppp: Correctly clean up PPP PCB after close. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- extmod/network_ppp_lwip.c | 19 ++++++++++--------- ports/esp32/network_ppp.c | 18 +++++++++--------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/extmod/network_ppp_lwip.c b/extmod/network_ppp_lwip.c index 883275f481..12205521f6 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 3a9cb239db..18e0c88168 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;