diff --git a/FORK_PRS.md b/FORK_PRS.md index 60e6dd9..8d3427d 100644 --- a/FORK_PRS.md +++ b/FORK_PRS.md @@ -19,8 +19,8 @@ become its own scoped PR. Issue numbers filled in once created. | # | Issue | Source fork | Commit(s) | Summary | Tier | |---|-------|-------------|-----------|---------|------| | 1 | ✅ [#14](https://github.com/fugo101/microlink/issues/14) (done) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `38602ab0`, `b25b1eee` | DERP TLS context leak on every failed `ml_derp_connect()` (~3-8KB/attempt, verified on hardware). Pre-existing issue filed 2026-08-17 from `UPSTREAM_PRS.md`'s "mineable from #22" note; enriched here with the cplewes source instead of building `derp_tls_abort()` from scratch. Adapted (not literal cherry-pick, our fork lacks entropy/ctr_drbg fields) into `ml_derp.c`'s `derp_free_tls_state()` + `fail_tls` goto path. | 1 | -| 2 | [#21](https://github.com/fugo101/microlink/issues/21) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `a415d646` | Teardown UAF: `microlink_stop()`/`destroy()` didn't join worker tasks before freeing context; replaces "sleep 3s and hope" with a real per-task liveness bitmask + reap-orphans path | 1 | -| 3 | [#22](https://github.com/fugo101/microlink/issues/22) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `7120dfa4` | Three crash sites triggered by captive-portal DNS failures: DERP double-init dangling pointer, `ml_coord_task` touching a freed event group post-destroy, corrupt-state races during parallel teardown | 1 | +| 2 | ✅ [#21](https://github.com/fugo101/microlink/issues/21) (done) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `a415d646` | Teardown UAF: `microlink_stop()`/`destroy()` didn't join worker tasks before freeing context; replaces "sleep 3s and hope" with a real per-task liveness bitmask + reap-orphans path | 1 | +| 3 | ✅ [#22](https://github.com/fugo101/microlink/issues/22) (done) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `7120dfa4` | Three crash sites triggered by captive-portal DNS failures: DERP double-init dangling pointer, `ml_coord_task` touching a freed event group post-destroy, corrupt-state races during parallel teardown. Landed together with #21 in the same PR — `a415d646` (the commit for #21) supersedes most of `7120dfa4`'s NULL-guard approach with a proper liveness bitmask; only the DERP mbedTLS double-init fix from `7120dfa4` was a distinct, still-needed piece. | 1 | | 4 | [#23](https://github.com/fugo101/microlink/issues/23) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `8f2ff39b`, `5c7303b4` | Captive-portal detection (HTTP 302 / TCP close) in `ml_coord.c`, backs off 5 min instead of retrying every 16s | 1 | | 5 | [#24](https://github.com/fugo101/microlink/issues/24) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `9f6af750` | Yield 1 tick per peer in `disco_periodic_probes` — prevents starving other same-core tasks on large tailnets | 1 | | 6 | [#25](https://github.com/fugo101/microlink/issues/25) | [`cplewes/microlink`](https://github.com/cplewes/microlink) | `b9636816` | DERP TLS session resumption (skip full ECDHE handshake on reconnect, ~7.5s→sub-second), connect timeout 10s→25s, per-peer exponential backoff on direct-path upgrade probes | 1 | diff --git a/components/microlink/include/microlink.h b/components/microlink/include/microlink.h index 72feacb..ad0f497 100644 --- a/components/microlink/include/microlink.h +++ b/components/microlink/include/microlink.h @@ -143,18 +143,39 @@ esp_err_t microlink_rebind(microlink_t *ml); /** * @brief Stop and disconnect from Tailscale * @param ml Handle - * @return ESP_OK on success + * @return ESP_OK once every worker task has exited, + * ESP_ERR_TIMEOUT if one or more were still running when the join + * window expired (the instance is still safe to destroy — see + * microlink_destroy()). * - * Gracefully shuts down all tasks and closes connections. + * Sets the shutdown request and waits for the worker tasks to acknowledge it + * by exiting. A worker wedged in a DNS lookup or TLS handshake on a captive + * network can outlast the window; that is reported, not fatal. */ esp_err_t microlink_stop(microlink_t *ml); /** * @brief Destroy MicroLink instance and free all resources * @param ml Handle (NULL-safe) + * + * If a worker task outlived microlink_stop()'s join window the instance is + * NOT freed here — freeing it would be a use-after-free the moment that task + * touched the context again. It is parked instead, and released by a later + * microlink_reap_orphans() / microlink_init() / microlink_destroy() call once + * the task has actually exited. The caller drops its handle either way. */ void microlink_destroy(microlink_t *ml); +/** + * @brief Release any instance whose free microlink_destroy() had to defer + * + * Called automatically by microlink_init() and microlink_destroy(). Exposed + * for applications that tear MicroLink down and then stay idle for a while + * and want the memory back sooner. Safe to call from any task at any time; + * it frees nothing that is still in use. + */ +void microlink_reap_orphans(void); + /** * @brief Get current connection state */ diff --git a/components/microlink/include/microlink_internal.h b/components/microlink/include/microlink_internal.h index 1c10f47..4b670e9 100644 --- a/components/microlink/include/microlink_internal.h +++ b/components/microlink/include/microlink_internal.h @@ -180,6 +180,24 @@ typedef struct { #define ML_EVT_DERP_RECONNECT BIT7 #define ML_EVT_DERP_CONNECT_REQ BIT8 +/* ============================================================================ + * Worker Task Liveness + * + * One bit per task created by microlink_start(). The bit is set BEFORE + * xTaskCreate and cleared by the task itself, as its very last action, via + * ml_task_exit(). A zero mask is therefore *proof* that no worker can touch + * the context again — which is what microlink_destroy() gates the free on. + * See the "Teardown contract" comment at the top of microlink.c. + * Adapted from cplewes/microlink@a415d646. + * ========================================================================== */ + +#define ML_TASK_BIT_NET_IO (1u << 0) +#define ML_TASK_BIT_DERP_TX (1u << 1) +#define ML_TASK_BIT_COORD (1u << 2) +#define ML_TASK_BIT_WG_MGR (1u << 3) +#define ML_TASK_BIT_ALL (ML_TASK_BIT_NET_IO | ML_TASK_BIT_DERP_TX | \ + ML_TASK_BIT_COORD | ML_TASK_BIT_WG_MGR) + /* ============================================================================ * Queue Message Types * ========================================================================== */ @@ -347,6 +365,11 @@ struct microlink_s { TaskHandle_t coord_task; TaskHandle_t wg_mgr_task; + /* ML_TASK_BIT_* mask of workers that are still running. Set before + * xTaskCreate, cleared by each task's ml_task_exit(). Touched from both + * cores — access only through the __atomic helpers in microlink.c. */ + uint32_t tasks_running; + /* Queues */ QueueHandle_t derp_tx_queue; /* -> derp_tx task */ QueueHandle_t disco_rx_queue; /* net_io -> wg_mgr */ @@ -454,6 +477,23 @@ struct microlink_s { * Internal Function Declarations (per-module) * ========================================================================== */ +/* microlink.c — worker task lifecycle helpers */ + +/** Current ML_TASK_BIT_* mask of running workers (0 = all joined). */ +uint32_t ml_tasks_running(const microlink_t *ml); + +/** True once ML_EVT_SHUTDOWN_REQUEST is set (or the context is gone). + * Call this from anywhere a worker can block for more than a moment — a + * DNS lookup, a TLS handshake retry, a backoff delay — so teardown does not + * have to wait out the full network timeout. */ +bool ml_shutdown_pending(microlink_t *ml); + +/** Terminate the calling worker task. Clears its ML_TASK_BIT_* and never + * returns. THE CONTEXT MUST NOT BE TOUCHED AFTER CALLING THIS — clearing the + * bit is what licenses microlink_destroy() to free ml, its queues and its + * event group. */ +void ml_task_exit(microlink_t *ml, uint32_t task_bit); + /* ml_net_io.c */ void ml_net_io_task(void *arg); diff --git a/components/microlink/src/microlink.c b/components/microlink/src/microlink.c index 851400b..8d1a4b9 100644 --- a/components/microlink/src/microlink.c +++ b/components/microlink/src/microlink.c @@ -12,6 +12,7 @@ #include "esp_timer.h" #include "esp_mac.h" #include "esp_wifi.h" +#include "esp_heap_caps.h" #include "nvs_flash.h" #include "nvs.h" #include "cJSON.h" @@ -163,6 +164,169 @@ esp_err_t microlink_factory_reset(void) { return ESP_OK; } +/* ============================================================================ + * Teardown contract + * + * The four worker tasks (net_io, derp_tx, coord, wg_mgr) hold a raw pointer + * to the context for their whole life, so the context may only be freed once + * every one of them has *provably* stopped running. Proof is the + * ml->tasks_running bitmask: microlink_start() sets a task's bit before + * creating it, and the task clears its own bit from ml_task_exit() as its + * final act, after which it touches nothing owned by the context. + * + * This replaces the previous "set the shutdown bit, sleep 3 s, assume they + * are gone" teardown. A worker blocked in getaddrinfo() or an mbedTLS + * handshake on a captive-portal network routinely outlives any fixed sleep; + * when it woke up it found ml, ml->events and the DERP TX queue already freed + * and reused, producing use-after-free crashes on real hardware. + * + * Waiting forever is not an option either — the caller is usually trying to + * bring a *new* instance up. So a stop that fails to join hands the context + * to the orphan list instead: nothing is freed, the stale workers keep + * running against memory that stays valid, and whichever of + * microlink_init() / microlink_destroy() / microlink_reap_orphans() runs + * next releases it once the mask finally reaches zero. Deferring a free is + * cheap; a use-after-free is a panic and a reboot. + * Adapted from cplewes/microlink@a415d646. + * ========================================================================== */ + +/* How long microlink_stop() waits for the workers before giving up on the + * join and letting the context be orphaned. Long enough to cover a DERP TLS + * handshake timing out (10 s SO_RCVTIMEO) but not so long that a reconnect + * stalls behind it. */ +#ifndef ML_STOP_JOIN_TIMEOUT_MS +#define ML_STOP_JOIN_TIMEOUT_MS 10000 +#endif +#define ML_JOIN_POLL_MS 20 + +/* Contexts that could not be freed at destroy time, awaiting their last + * worker. One slot per outstanding teardown; in practice never more than + * one, since each teardown is followed by a reap. */ +#define ML_MAX_ORPHANS 4 +static microlink_t *s_orphans[ML_MAX_ORPHANS]; +static portMUX_TYPE s_orphan_mux = portMUX_INITIALIZER_UNLOCKED; + +static void ml_release(microlink_t *ml); + +uint32_t ml_tasks_running(const microlink_t *ml) { + if (!ml) return 0; + return __atomic_load_n(&ml->tasks_running, __ATOMIC_ACQUIRE); +} + +bool ml_shutdown_pending(microlink_t *ml) { + if (!ml || !ml->events) return true; + return (xEventGroupGetBits(ml->events) & ML_EVT_SHUTDOWN_REQUEST) != 0; +} + +void ml_task_exit(microlink_t *ml, uint32_t task_bit) { + if (ml) { + __atomic_fetch_and(&ml->tasks_running, ~task_bit, __ATOMIC_RELEASE); + } + /* ml, ml->events and every ml-owned queue are off limits from here on: + * clearing the bit above is exactly what licenses another task to free + * them, possibly before the vTaskDelete() below has even run. */ + vTaskDelete(NULL); + for (;;) { vTaskDelay(portMAX_DELAY); } /* not reached */ +} + +static void ml_describe_tasks(uint32_t mask, char *out, size_t out_len) { + snprintf(out, out_len, "%s%s%s%s", + (mask & ML_TASK_BIT_NET_IO) ? " net_io" : "", + (mask & ML_TASK_BIT_DERP_TX) ? " derp_tx" : "", + (mask & ML_TASK_BIT_COORD) ? " coord" : "", + (mask & ML_TASK_BIT_WG_MGR) ? " wg_mgr" : ""); +} + +/* Poll the liveness mask until it clears or the deadline passes. Returns the + * mask of tasks still running (0 == fully joined). */ +static uint32_t ml_join_tasks(microlink_t *ml, uint32_t timeout_ms) { + uint32_t waited_ms = 0; + uint32_t alive; + + while ((alive = ml_tasks_running(ml)) != 0 && waited_ms < timeout_ms) { + vTaskDelay(pdMS_TO_TICKS(ML_JOIN_POLL_MS)); + waited_ms += ML_JOIN_POLL_MS; + } + return alive; +} + +static void ml_orphan_park(microlink_t *ml) { + int slot = -1; + + portENTER_CRITICAL(&s_orphan_mux); + for (int i = 0; i < ML_MAX_ORPHANS; i++) { + if (!s_orphans[i]) { s_orphans[i] = ml; slot = i; break; } + } + portEXIT_CRITICAL(&s_orphan_mux); + + if (slot < 0) { + /* Every slot taken means several teardowns in a row all left workers + * behind — the network is badly wedged. Leaking permanently is still + * the correct trade against a use-after-free. */ + ESP_LOGE(TAG, "orphan table full; context %p leaked permanently", ml); + } else { + ESP_LOGW(TAG, "context %p parked in orphan slot %d until its workers exit", + ml, slot); + } +} + +void microlink_reap_orphans(void) { + for (int i = 0; i < ML_MAX_ORPHANS; i++) { + microlink_t *ripe = NULL; + + portENTER_CRITICAL(&s_orphan_mux); + if (s_orphans[i] && ml_tasks_running(s_orphans[i]) == 0) { + ripe = s_orphans[i]; + s_orphans[i] = NULL; + } + portEXIT_CRITICAL(&s_orphan_mux); + + if (ripe) { + ESP_LOGW(TAG, "reaping orphaned context %p (workers finally exited)", ripe); + ml_release(ripe); + } + } +} + +/* Free everything the context owns. Precondition: ml_tasks_running(ml) == 0. + * Called either straight from microlink_destroy() or later from the reaper. */ +static void ml_release(microlink_t *ml) { + /* Deinitialize HTTP config server */ + if (ml->config_httpd) { + ml_config_httpd_deinit(ml->config_httpd); + ml->config_httpd = NULL; + } + + /* Sockets microlink_stop() had to leave open because a worker was still + * inside select()/recv() on them, plus the per-task sockets if their + * owner exited without getting to its own cleanup. */ +#ifdef CONFIG_ML_ZERO_COPY_WG + ml_zerocopy_deinit(ml); +#endif + if (ml->disco_sock4 >= 0) { ml_close_sock(ml->disco_sock4); ml->disco_sock4 = -1; } + if (ml->stun_sock >= 0) { ml_close_sock(ml->stun_sock); ml->stun_sock = -1; } + if (ml->stun_sock6 >= 0) { ml_close_sock(ml->stun_sock6); ml->stun_sock6 = -1; } + if (ml->coord_sock >= 0) { ml_close_sock(ml->coord_sock); ml->coord_sock = -1; } + if (ml->derp.sockfd >= 0) { ml_close_sock(ml->derp.sockfd); ml->derp.sockfd = -1; } + + if (ml->derp_tx_queue) { vQueueDelete(ml->derp_tx_queue); ml->derp_tx_queue = NULL; } + if (ml->disco_rx_queue) { vQueueDelete(ml->disco_rx_queue); ml->disco_rx_queue = NULL; } + if (ml->wg_rx_queue) { vQueueDelete(ml->wg_rx_queue); ml->wg_rx_queue = NULL; } + if (ml->stun_rx_queue) { vQueueDelete(ml->stun_rx_queue); ml->stun_rx_queue = NULL; } + if (ml->coord_cmd_queue) { vQueueDelete(ml->coord_cmd_queue); ml->coord_cmd_queue = NULL; } + if (ml->peer_update_queue){ vQueueDelete(ml->peer_update_queue);ml->peer_update_queue= NULL; } + + if (ml->events) { vEventGroupDelete(ml->events); ml->events = NULL; } + + /* Clear keys from memory */ + memset(ml->machine_private_key, 0, 32); + memset(ml->wg_private_key, 0, 32); + memset(ml->disco_private_key, 0, 32); + + free(ml); + ESP_LOGI(TAG, "Destroyed"); +} + /* ============================================================================ * Public API * ========================================================================== */ @@ -173,6 +337,10 @@ microlink_t *microlink_init(const microlink_config_t *config) { return NULL; } + /* Reclaim any earlier context whose workers have drained since the last + * teardown, before allocating another one. */ + microlink_reap_orphans(); + /* Route cJSON to PSRAM */ cJSON_Hooks hooks = { .malloc_fn = cjson_psram_malloc, @@ -319,8 +487,20 @@ esp_err_t microlink_start(microlink_t *ml) { ESP_LOGW(TAG, "Already started (state=%d)", ml->state); return ESP_ERR_INVALID_STATE; } + /* Restarting a context whose previous workers are still draining would + * give two generations of tasks the same liveness bits, and the first one + * to exit would clear the bit the other is still relying on. Make the + * caller build a fresh context instead. */ + uint32_t stragglers = ml_tasks_running(ml); + if (stragglers) { + char names[48]; + ml_describe_tasks(stragglers, names, sizeof(names)); + ESP_LOGE(TAG, "Refusing to start: previous workers still running:%s", names); + return ESP_ERR_INVALID_STATE; + } ml->state = ML_STATE_WIFI_WAIT; + xEventGroupClearBits(ml->events, ML_EVT_SHUTDOWN_REQUEST); /* Set WiFi TX power if configured */ if (ml->config.wifi_tx_power_dbm > 0) { @@ -383,36 +563,37 @@ esp_err_t microlink_start(microlink_t *ml) { ; #endif - /* Create tasks */ + /* Create tasks. If any create fails (most commonly OOM in internal RAM + * when overall heap is low), we MUST clean up any tasks that already + * came up before returning — otherwise they become orphans accessing + * state that microlink_destroy() will later free. Each bit goes up + * BEFORE its xTaskCreate: from the instant the task exists it may be + * holding the context, and only it may clear the bit again. On a create + * failure we take the bit back down here — nobody else can, because no + * task was born to do it. */ BaseType_t ret; + const char *failed_task = NULL; + uint32_t failed_bit = 0; + __atomic_fetch_or(&ml->tasks_running, ML_TASK_BIT_NET_IO, __ATOMIC_RELEASE); ret = xTaskCreatePinnedToCore(ml_net_io_task, "ml_net_io", ML_TASK_NET_IO_STACK, ml, ML_TASK_NET_IO_PRIO, &ml->net_io_task, ML_TASK_NET_IO_CORE); - if (ret != pdPASS) { - ESP_LOGE(TAG, "Failed to create net_io task"); - return ESP_FAIL; - } + if (ret != pdPASS) { failed_task = "ml_net_io"; failed_bit = ML_TASK_BIT_NET_IO; goto fail_start; } + __atomic_fetch_or(&ml->tasks_running, ML_TASK_BIT_DERP_TX, __ATOMIC_RELEASE); ret = xTaskCreatePinnedToCore(ml_derp_tx_task, "ml_derp_tx", ML_TASK_DERP_TX_STACK, ml, ML_TASK_DERP_TX_PRIO, &ml->derp_tx_task, ML_TASK_DERP_TX_CORE); - if (ret != pdPASS) { - ESP_LOGE(TAG, "Failed to create derp_tx task"); - return ESP_FAIL; - } + if (ret != pdPASS) { failed_task = "ml_derp_tx"; failed_bit = ML_TASK_BIT_DERP_TX; goto fail_start; } + __atomic_fetch_or(&ml->tasks_running, ML_TASK_BIT_COORD, __ATOMIC_RELEASE); ret = xTaskCreatePinnedToCore(ml_coord_task, "ml_coord", ML_TASK_COORD_STACK, ml, ML_TASK_COORD_PRIO, &ml->coord_task, ML_TASK_COORD_CORE); - if (ret != pdPASS) { - ESP_LOGE(TAG, "Failed to create coord task"); - return ESP_FAIL; - } + if (ret != pdPASS) { failed_task = "ml_coord"; failed_bit = ML_TASK_BIT_COORD; goto fail_start; } + __atomic_fetch_or(&ml->tasks_running, ML_TASK_BIT_WG_MGR, __ATOMIC_RELEASE); ret = xTaskCreatePinnedToCore(ml_wg_mgr_task, "ml_wg_mgr", ML_TASK_WG_MGR_STACK, ml, ML_TASK_WG_MGR_PRIO, &ml->wg_mgr_task, ML_TASK_WG_MGR_CORE); - if (ret != pdPASS) { - ESP_LOGE(TAG, "Failed to create wg_mgr task"); - return ESP_FAIL; - } + if (ret != pdPASS) { failed_task = "ml_wg_mgr"; failed_bit = ML_TASK_BIT_WG_MGR; goto fail_start; } /* WiFi is expected to be connected before microlink_start() is called. * Signal the event so coord/wg_mgr tasks proceed immediately. */ @@ -429,6 +610,27 @@ esp_err_t microlink_start(microlink_t *ml) { ESP_LOGI(TAG, "All tasks started"); return ESP_OK; + +fail_start: + /* A task creation failed. The tasks we DID manage to create are now + * running and would otherwise orphan-access state on the next destroy. + * Signal shutdown and let microlink_stop() join them before returning to + * the caller, so the partial-init goes back to a clean ML_STATE_IDLE that + * the caller can microlink_destroy() safely. + * + * Diagnostic dump tells you which task fell over and how starved heap + * was at the moment of failure — usually internal RAM exhaustion. */ + __atomic_fetch_and(&ml->tasks_running, ~failed_bit, __ATOMIC_RELEASE); + ESP_LOGE(TAG, "Failed to create %s task (free internal=%u psram=%u); " + "rolling back partial init", + failed_task, + (unsigned)heap_caps_get_free_size(MALLOC_CAP_INTERNAL), + (unsigned)heap_caps_get_free_size(MALLOC_CAP_SPIRAM)); + /* microlink_stop() sets the shutdown bit and joins whatever came up. If + * one of them is wedged it stays in the liveness mask and destroy will + * defer the free rather than pull the context out from under it. */ + microlink_stop(ml); + return ESP_FAIL; } esp_err_t microlink_rebind(microlink_t *ml) { @@ -525,38 +727,54 @@ esp_err_t microlink_stop(microlink_t *ml) { if (!ml) return ESP_ERR_INVALID_ARG; ESP_LOGI(TAG, "Stopping..."); - xEventGroupSetBits(ml->events, ML_EVT_SHUTDOWN_REQUEST); + if (ml->events) { + xEventGroupSetBits(ml->events, ML_EVT_SHUTDOWN_REQUEST); + } + + /* Join the workers for real. Each one clears its ML_TASK_BIT_* as its + * last act, so a zero mask proves none of them can reach this context + * again — see the teardown contract above. Tasks self-delete, so we + * never call vTaskDelete() on them from here. */ + uint32_t alive = ml_join_tasks(ml, ML_STOP_JOIN_TIMEOUT_MS); + char names[48]; + ml_describe_tasks(alive, names, sizeof(names)); - /* Wait for tasks to exit (they check ML_EVT_SHUTDOWN_REQUEST). - * Tasks call vTaskDelete(NULL) to self-delete, so we must NOT call - * vTaskDelete() on them again — that causes a crash in uxListRemove - * because the task's list node is already invalid. Just wait and - * NULL the handles. */ - vTaskDelay(pdMS_TO_TICKS(3000)); + if (alive) { + ESP_LOGE(TAG, "Join timed out after %d ms; still running:%s (mask 0x%02x). " + "Context will be held until they exit.", + ML_STOP_JOIN_TIMEOUT_MS, names, (unsigned)alive); + } - ml->net_io_task = NULL; - ml->derp_tx_task = NULL; - ml->coord_task = NULL; - ml->wg_mgr_task = NULL; + /* A handle is only meaningful while its task exists; clear the ones we + * have proof are gone and keep the rest for diagnostics. */ + if (!(alive & ML_TASK_BIT_NET_IO)) ml->net_io_task = NULL; + if (!(alive & ML_TASK_BIT_DERP_TX)) ml->derp_tx_task = NULL; + if (!(alive & ML_TASK_BIT_COORD)) ml->coord_task = NULL; + if (!(alive & ML_TASK_BIT_WG_MGR)) ml->wg_mgr_task = NULL; /* Stop HTTP config server */ if (ml->config_httpd) { ml_config_httpd_stop(ml->config_httpd); } - /* Clean up zero-copy PCB if active */ + /* The DISCO/STUN sockets are the ones net_io holds in select(); closing + * a descriptor out from under a live select() is the lwIP deadlock that + * microlink_rebind() goes out of its way to avoid, so only close them + * once net_io is provably gone. Freeing them promptly matters — the next + * instance wants to bind port 51820 again. If net_io is the straggler, + * ml_release() closes them at reap time instead. */ + if (!(alive & ML_TASK_BIT_NET_IO)) { #ifdef CONFIG_ML_ZERO_COPY_WG - ml_zerocopy_deinit(ml); + ml_zerocopy_deinit(ml); #endif - - /* Close sockets */ - if (ml->disco_sock4 >= 0) { ml_close_sock(ml->disco_sock4); ml->disco_sock4 = -1; } - if (ml->stun_sock >= 0) { ml_close_sock(ml->stun_sock); ml->stun_sock = -1; } - if (ml->stun_sock6 >= 0) { ml_close_sock(ml->stun_sock6); ml->stun_sock6 = -1; } + if (ml->disco_sock4 >= 0) { ml_close_sock(ml->disco_sock4); ml->disco_sock4 = -1; } + if (ml->stun_sock >= 0) { ml_close_sock(ml->stun_sock); ml->stun_sock = -1; } + if (ml->stun_sock6 >= 0) { ml_close_sock(ml->stun_sock6); ml->stun_sock6 = -1; } + } ml->state = ML_STATE_IDLE; - ESP_LOGI(TAG, "Stopped"); - return ESP_OK; + ESP_LOGI(TAG, "Stopped%s", alive ? " (workers still draining)" : ""); + return alive ? ESP_ERR_TIMEOUT : ESP_OK; } void microlink_destroy(microlink_t *ml) { @@ -564,33 +782,26 @@ void microlink_destroy(microlink_t *ml) { microlink_stop(ml); - /* Deinitialize peer NVS */ - ml_peer_nvs_deinit(); + /* Opportunistic: release anything parked by an earlier teardown that has + * drained in the meantime. */ + microlink_reap_orphans(); - /* Deinitialize HTTP config server */ - if (ml->config_httpd) { - ml_config_httpd_deinit(ml->config_httpd); - ml->config_httpd = NULL; + uint32_t alive = ml_tasks_running(ml); + if (alive) { + /* Freeing now is precisely the bug this contract exists to prevent: + * the straggler wakes up from its socket call and dereferences ml, + * ml->events or the DERP TX queue. Park the context instead. */ + char names[48]; + ml_describe_tasks(alive, names, sizeof(names)); + ESP_LOGE(TAG, "Deferring free:%s still running (mask 0x%02x)", names, (unsigned)alive); + ml_orphan_park(ml); + return; } - /* Delete queues */ - if (ml->derp_tx_queue) vQueueDelete(ml->derp_tx_queue); - if (ml->disco_rx_queue) vQueueDelete(ml->disco_rx_queue); - if (ml->wg_rx_queue) vQueueDelete(ml->wg_rx_queue); - if (ml->stun_rx_queue) vQueueDelete(ml->stun_rx_queue); - if (ml->coord_cmd_queue) vQueueDelete(ml->coord_cmd_queue); - if (ml->peer_update_queue) vQueueDelete(ml->peer_update_queue); - - /* Delete event group */ - if (ml->events) vEventGroupDelete(ml->events); - - /* Clear keys from memory */ - memset(ml->machine_private_key, 0, 32); - memset(ml->wg_private_key, 0, 32); - memset(ml->disco_private_key, 0, 32); - - free(ml); - ESP_LOGI(TAG, "Destroyed"); + /* Peer NVS is a process-wide handle, not per-instance — only close it on + * the path where this instance really is the last one standing. */ + ml_peer_nvs_deinit(); + ml_release(ml); } /* ============================================================================ diff --git a/components/microlink/src/ml_coord.c b/components/microlink/src/ml_coord.c index d5c5865..6037554 100644 --- a/components/microlink/src/ml_coord.c +++ b/components/microlink/src/ml_coord.c @@ -2175,7 +2175,7 @@ void ml_coord_task(void *arg) { pdFALSE, pdFALSE, portMAX_DELAY); if (wb & ML_EVT_SHUTDOWN_REQUEST) { ESP_LOGI(TAG, "Shutdown requested before WiFi, exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_COORD); return; } } @@ -2643,5 +2643,5 @@ void ml_coord_task(void *arg) { memset(&noise, 0, sizeof(noise)); ESP_LOGI(TAG, "Coord task exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_COORD); } diff --git a/components/microlink/src/ml_derp.c b/components/microlink/src/ml_derp.c index 37a27ac..d36ac2f 100644 --- a/components/microlink/src/ml_derp.c +++ b/components/microlink/src/ml_derp.c @@ -523,7 +523,10 @@ void ml_derp_tx_task(void *arg) { EventBits_t bits = xEventGroupGetBits(ml->events); if ((bits & ML_EVT_DERP_CONNECT_REQ) && !ml->derp.connected) { xEventGroupClearBits(ml->events, ML_EVT_DERP_CONNECT_REQ); - /* Retry up to 3 times with 2s backoff */ + /* Retry up to 3 times with 2s backoff. Each attempt can cost + * a DNS timeout plus a 10s TLS handshake, so re-check for + * shutdown between them — three blind retries is how this + * task used to outlive microlink_stop()'s join window. */ for (int attempt = 0; attempt < 3 && !ml->derp.connected; attempt++) { if (attempt > 0) { ESP_LOGW(TAG, "DERP connect retry %d/3 in 2s...", attempt + 1); @@ -531,6 +534,10 @@ void ml_derp_tx_task(void *arg) { } else { ESP_LOGI(TAG, "DERP connect requested, connecting from I/O task"); } + if (ml_shutdown_pending(ml)) { + ESP_LOGI(TAG, "Shutdown during DERP connect; abandoning retries"); + break; + } if (ml_derp_connect(ml) == ESP_OK) { connected_since_ms = ml_get_time_ms(); verbose_phase = true; @@ -552,6 +559,10 @@ void ml_derp_tx_task(void *arg) { ESP_LOGW(TAG, "DERP reconnect retry %d/3 in 2s...", attempt + 1); vTaskDelay(pdMS_TO_TICKS(2000)); } + if (ml_shutdown_pending(ml)) { + ESP_LOGI(TAG, "Shutdown during DERP reconnect; abandoning retries"); + break; + } if (ml_derp_connect(ml) == ESP_OK) { connected_since_ms = ml_get_time_ms(); verbose_phase = true; @@ -630,8 +641,13 @@ void ml_derp_tx_task(void *arg) { vTaskDelay(pdMS_TO_TICKS(1)); } + /* Release the TLS state, the socket and anything still queued for TX. + * Nobody else can: microlink_stop() deliberately leaves derp.sockfd to + * this task, and on the orphan path the context outlives the stop. */ + ml_derp_disconnect(ml); + ESP_LOGI(TAG, "DERP I/O task exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_DERP_TX); } /* ============================================================================ @@ -655,6 +671,13 @@ static void derp_free_tls_state(microlink_t *ml) { } esp_err_t ml_derp_connect(microlink_t *ml) { + /* Don't start a fresh DNS + TCP + TLS sequence the caller is about to + * throw away — every phase below can block for seconds. */ + if (ml_shutdown_pending(ml)) { + ESP_LOGI(TAG, "Shutdown requested; not connecting to DERP"); + return ESP_FAIL; + } + /* Determine DERP host/port from DERPMap with node failover. * Always start from node 0 (the first/preferred node in the DERPMap). * Only rotate to a different node after a SUCCESSFUL connection drops, @@ -755,7 +778,23 @@ esp_err_t ml_derp_connect(microlink_t *ml) { int64_t t_derp_tcp = esp_timer_get_time(); ESP_LOGI(TAG, "[TIMING] DERP TCP connect: %lld ms", (t_derp_tcp - t_derp_dns) / 1000); - /* TLS setup */ + /* TLS setup. CRITICAL: free any prior SSL state before re-init. + * mbedtls_ssl_init() zeroes the struct without freeing internal heap + * allocations; if a previous ml_derp_connect() got partway through the + * setup calls below before failing (e.g. mid-retry on a captive-portal + * network with blocked DNS), those buffers are orphaned and this re-init + * loses the only pointers to them. Freeing first avoids both the leak + * and a dangling-pointer reuse in mbedTLS's internal state on the next + * handshake. mbedtls_ssl_free/config_free are safe to call on an + * already-zeroed or already-freed struct — they check internal sentinel + * fields before deref'ing. No entropy/ctr_drbg free here: those fields + * don't exist in our ml_derp_conn_t (RNG is PSA-owned post mbedTLS 4.x + * migration, see ESP_IDF_6X_COMPAT.md) — same adaptation as + * derp_free_tls_state() above. + * Adapted from cplewes/microlink@7120dfa4. */ + mbedtls_ssl_free(&ml->derp.ssl); + mbedtls_ssl_config_free(&ml->derp.ssl_conf); + mbedtls_ssl_init(&ml->derp.ssl); mbedtls_ssl_config_init(&ml->derp.ssl_conf); @@ -803,6 +842,14 @@ esp_err_t ml_derp_connect(microlink_t *ml) { int ret; while ((ret = mbedtls_ssl_handshake(&ml->derp.ssl)) != 0) { if (ret == MBEDTLS_ERR_SSL_WANT_READ || ret == MBEDTLS_ERR_SSL_WANT_WRITE) { + /* This spin is where the task can sit for tens of seconds on a + * captive network — each pass costs a full socket read timeout. + * Bail out on shutdown so the teardown join doesn't have to wait + * for the server to answer; fail_tls frees the mbedTLS state. */ + if (ml_shutdown_pending(ml)) { + ESP_LOGW(TAG, "Shutdown during TLS handshake; aborting"); + goto fail_tls; + } continue; } char err_buf[128]; diff --git a/components/microlink/src/ml_net_io.c b/components/microlink/src/ml_net_io.c index f8b6799..9711cb2 100644 --- a/components/microlink/src/ml_net_io.c +++ b/components/microlink/src/ml_net_io.c @@ -209,5 +209,5 @@ void ml_net_io_task(void *arg) { } ESP_LOGI(TAG, "Net I/O task exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_NET_IO); } diff --git a/components/microlink/src/ml_wg_mgr.c b/components/microlink/src/ml_wg_mgr.c index e64fd8a..1e59239 100644 --- a/components/microlink/src/ml_wg_mgr.c +++ b/components/microlink/src/ml_wg_mgr.c @@ -1526,7 +1526,7 @@ void ml_wg_mgr_task(void *arg) { if (wait_bits & ML_EVT_SHUTDOWN_REQUEST) { ESP_LOGI(TAG, "Shutdown requested before registration, exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_WG_MGR); return; /* Not reached */ } @@ -1660,5 +1660,5 @@ void ml_wg_mgr_task(void *arg) { } ESP_LOGI(TAG, "WG Manager task exiting"); - vTaskDelete(NULL); + ml_task_exit(ml, ML_TASK_BIT_WG_MGR); }