From 6ad6c00d68a1bf03db940ee4ce4c6a8765bce39e Mon Sep 17 00:00:00 2001 From: Commander1024 Date: Mon, 31 Aug 2026 03:33:43 +0200 Subject: [PATCH] Harden cleanup and reduce internal RAM use. Phase 8C nearly validated and somewhat stable. --- docs/agent/architecture.md | 2 +- docs/agent/code-map.md | 4 +- docs/agent/current-state.md | 11 +++- docs/agent/design-decisions.md | 4 +- src/admin_ssh_console.c | 3 +- src/console_completion.c | 2 +- src/console_completion.h | 3 ++ src/serial_service.c | 35 ++++++++++-- src/ssh_transport.c | 18 +++++-- src/usb_cdc_transport.c | 46 ++++++++++------ src/user_database.c | 98 ++++++++++++++++++++++------------ src/web_server.c | 11 +++- 12 files changed, 169 insertions(+), 68 deletions(-) diff --git a/docs/agent/architecture.md b/docs/agent/architecture.md index c73e343..9b60779 100644 --- a/docs/agent/architecture.md +++ b/docs/agent/architecture.md @@ -190,5 +190,5 @@ Hardware diagnostics are synchronous console commands. RS-232 tests own the phys - Library/hardware ownership is centralized: serial task owns UART1 while running, display service owns I2C/framebuffer, the SSH owner task owns post-initialization wolfSSH runtime calls, and the console dispatcher owns `esp_console_run()`. - Password authentication performs PBKDF2 outside the user-database mutex and revalidates afterward. Some password mutation paths currently derive verifiers while holding the mutation lock; do not generalize the authentication locking pattern without checking the exact path. - Avoid holding service/database/broker locks across I2C, network sends, or other potentially long operations unless the existing contract explicitly requires it. Preserve the existing broker-before-serial lock order. -- Broker and selected cryptographic allocations prefer PSRAM but can fall back to internal RAM. FreeRTOS control structures and task stacks intentionally remain internal where flash/cache-disable safety matters. +- Serial RX/TX stream payloads, broker per-client payloads, the transactional user-database candidate, and selected cryptographic allocations prefer PSRAM with internal fallback. The live user database, FreeRTOS control structures, UART driver buffers, and task stacks remain internal where deterministic/cache-disable access matters. - The build disables wolfSSL ESP32 AES/SHA acceleration, and the HTTPS path uses software AES for PSRAM-backed records. This preserves the validated workaround for uncoordinated mbedTLS/wolfSSL hardware-crypto locks and a prior mbedTLS external-RAM DMA watchdog stall. diff --git a/docs/agent/code-map.md b/docs/agent/code-map.md index 0c15179..2dea91b 100644 --- a/docs/agent/code-map.md +++ b/docs/agent/code-map.md @@ -30,7 +30,7 @@ This is a semantic map, not a complete file inventory. Start here, then read the - Normal data caller: `session_broker`; USB, WebSocket, role-`user` SSH, console, and local UI also call serial lifecycle/configuration APIs as appropriate - Dependencies: ESP-IDF UART driver, `board_pins.h`, NVS - Ownership: the cooperative owner arbitrates active diagnostics (`PHASE0`) against the service (`SERVICE`); boot-time static-safe GPIO initialization and service-owned static-mode restoration are explicit exceptions. Unsafe cleanup marks `FAULT` until reboot. -- Lifecycle: stop/reconfiguration discards serial-service RX/TX and task-local pending bytes, but leaves broker clients, writer ownership, events, and already-fanned output intact. +- Lifecycle: stop/reconfiguration discards serial-service RX/TX and task-local pending bytes, but leaves broker clients, writer ownership, events, and already-fanned output intact. The 16 KiB RX and 8 KiB TX payloads prefer PSRAM; their FreeRTOS controls and UART driver storage remain internal. ## Session broker @@ -91,7 +91,7 @@ This is a semantic map, not a complete file inventory. Start here, then read the - Interfaces: init/migration/recovery, authenticate, principal-currentness, account/password/role/key mutations, snapshots - Called by: web and SSH authentication/currentness checks and console administration - Dependencies: NVS, secure random, mbedTLS cryptography; after a committed command-layer mutation, best-effort web/SSH revocation calls supplement authoritative transport currentness checks -- Ownership: database mutex protects live records; password authentication runs PBKDF2 outside it and revalidates afterward, while mutation locking must be checked per operation. +- Ownership: database mutex protects the internal live record and PSRAM-preferred transactional candidate; password authentication runs PBKDF2 outside the mutex and revalidates afterward, while mutation locking must be checked per operation. - Authorization: UART0 exclusively owns initial administrator bootstrap and unavailable-database recovery; current admins may use admin SSH for other commands unless handler policy denies them. HTTPS currently treats both roles alike. - Constraint: final administrator cannot be deleted or demoted; transport principals must be rechecked after mutations. diff --git a/docs/agent/current-state.md b/docs/agent/current-state.md index 9aa0b74..7f43f82 100644 --- a/docs/agent/current-state.md +++ b/docs/agent/current-state.md @@ -13,6 +13,15 @@ Based on checked-in source plus `README.md` and `docs/roadmap.md`: - Security/production hardening, OTA, BLE evaluation, advanced networking, and optional filesystem features remain future roadmap work. - Reserved OTA, coredump, NVS-key, and storage partitions do not imply those runtime features are implemented. +## Recent memory audit + +- Fixed failed-initialization ownership leaks for wolfSSH, partial HTTPS startup, and TinyUSB teardown. Failed teardown now retains ownership and blocks unsafe duplicate initialization. +- Serial-service RX/TX stream payloads (16 KiB and 8 KiB effective capacity) now prefer PSRAM with internal fallback; FreeRTOS controls and UART driver buffers remain internal. +- The 5,360-byte transactional user-database candidate now prefers PSRAM with internal fallback while the live database remains internal. Candidate contents are wiped after each transaction and wiped/freed on initialization or recovery failure. +- UART and admin-SSH completion formatter buffers were reduced from 2 KiB to 1 KiB each; current worst-case output is 890 bytes and overflow remains fail-closed. +- Linked RAM fell from 99,508 to 92,188 bytes (7,320 bytes). PSRAM placement of serial payloads additionally removes about 24 KiB of normal internal-heap pressure on the target. +- `pio run` passes. A preliminary target run reports significantly more free memory and stable, improved operation after these changes. This is useful evidence but not completion of Phase 8C validation. +- Remaining targeted checks include stored/migrated/recovered user-database mutations, USB enumeration, HTTPS start/stop failure recovery where injectable, SSH initialization/login, completion display, and sustained multi-transport serial traffic while checking `memory` telemetry. ## Clearly incomplete or transitional areas @@ -21,12 +30,10 @@ Based on checked-in source plus `README.md` and `docs/roadmap.md`: - Browser authentication still uses HTTP Basic; integrated login/logout sessions are planned. - NVS encryption, secure boot/flash encryption review, authentication rate limiting, production certificate/provisioning policy, and OTA are not implemented. - ## Known inconsistencies These observations should be checked when touching the relevant area; they are not automatically bugs requiring unrelated cleanup. - - Some source comments still call shared commands UART0-only or call the current local status/control task read-only. - `USER_DATABASE_LOAD_EMPTY` is only an initialization/failure sentinel at the checked-in revision: every successful `user_database_init()` path returns `STORED` or `MIGRATED_LEGACY`, so `main.c`'s successful "new empty" log branch is unreachable. - SSH startup is currently gated on successful `web_security` initialization even though SSH uses separate host-key material. **Needs verification:** whether this coupling is intentional recovery policy or an accidental startup dependency. diff --git a/docs/agent/design-decisions.md b/docs/agent/design-decisions.md index f2706a5..925be8d 100644 --- a/docs/agent/design-decisions.md +++ b/docs/agent/design-decisions.md @@ -96,9 +96,9 @@ Only constraints supported by implementation or current project documentation be **Decision:** Application settings, users, and identities use separate fixed/versioned NVS blobs. Serial, Wi-Fi, and local-UI working edits are RAM-only until explicitly saved. User mutations and HTTPS/SSH identity changes commit directly as part of the operation. Invalid ordinary configuration generally selects RAM defaults without erasing storage; malformed security material fails closed and needs explicit reset. -**Rationale/evidence:** Serial, Wi-Fi, local UI, web security, users, and SSH security each validate schema/size and own their namespace. User/security mutations build and validate candidate state before committing it; security modules avoid silently replacing an established identity. +**Rationale/evidence:** Serial, Wi-Fi, local UI, web security, users, and SSH security each validate schema/size and own their namespace. User/security mutations build and validate candidate state before committing it; security modules avoid silently replacing an established identity. The live user database remains internal while its 5,360-byte candidate is a persistent PSRAM-preferred allocation with internal fallback and is wiped after every transaction. -**Consequence for future changes:** Add schema versions and transactional candidate validation. Do not overwrite unknown records automatically; provide explicit migration/reset behavior. Preserve the distinct persistence contracts: explicit save/load/default/reset for working configuration and per-blob commit-before-live-install for user and identity mutation. Pre-bootstrap legacy credential rotation spans `web_sec/material` and `user_db/database`, is not cross-namespace atomic, and relies on boot reconciliation after interruption. +**Consequence for future changes:** Add schema versions and transactional candidate validation. Do not overwrite unknown records automatically; provide explicit migration/reset behavior. Preserve the distinct persistence contracts: explicit save/load/default/reset for working configuration and per-blob commit-before-live-install for user and identity mutation. Keep candidate ownership mutex-local and wipe/free it on initialization or recovery failure. Recheck external-buffer staging in the flash/NVS implementation when upgrading from the pinned ESP-IDF 5.5 baseline. Pre-bootstrap legacy credential rotation spans `web_sec/material` and `user_db/database`, is not cross-namespace atomic, and relies on boot reconciliation after interruption. **Relevant files:** `src/serial_config.c`, `src/wifi_config.c`, `src/local_ui_config.c`, `src/web_security.c`, `src/user_database.c`, `src/ssh_security.c` diff --git a/src/admin_ssh_console.c b/src/admin_ssh_console.c index 7bf7aaf..6961f00 100644 --- a/src/admin_ssh_console.c +++ b/src/admin_ssh_console.c @@ -29,7 +29,6 @@ #define ADMIN_UART_CONSOLE_TASK_PRIORITY 3U #define ADMIN_SSH_CONSOLE_MAX_ARGUMENTS 10U #define ADMIN_SSH_CONSOLE_HISTORY_DEPTH 4U -#define ADMIN_SSH_CONSOLE_COMPLETION_OUTPUT_CAPACITY 2048U #define ADMIN_SSH_CONTROL_QUEUE_LENGTH 2U #define ADMIN_SSH_CONTROL_TASK_STACK_SIZE 4096U #define ADMIN_SSH_CONTROL_TASK_PRIORITY 3U @@ -94,7 +93,7 @@ typedef struct { static portMUX_TYPE s_lock = portMUX_INITIALIZER_UNLOCKED; static admin_session_t s_sessions[ADMIN_SSH_CONSOLE_MAX_SESSIONS]; /* admin_ssh_console_feed_input() is called only by the sole SSH owner task. */ -static char s_completion_output[ADMIN_SSH_CONSOLE_COMPLETION_OUTPUT_CAPACITY]; +static char s_completion_output[CONSOLE_COMPLETION_OUTPUT_CAPACITY]; static StaticQueue_t s_request_queue_storage; static uint8_t s_request_queue_bytes[ADMIN_SSH_CONSOLE_REQUEST_QUEUE_LENGTH * diff --git a/src/console_completion.c b/src/console_completion.c index 6168413..4e81955 100644 --- a/src/console_completion.c +++ b/src/console_completion.c @@ -329,7 +329,7 @@ static ssize_t console_read_with_late_terminal_upgrade(int file_descriptor, } /* The UART frontend is the sole caller of linenoise's completion callback. */ -static char s_uart_completion_output[2048U]; +static char s_uart_completion_output[CONSOLE_COMPLETION_OUTPUT_CAPACITY]; static void console_completion_callback(const char *buffer, linenoiseCompletions *completions) { diff --git a/src/console_completion.h b/src/console_completion.h index b9908f8..72cc0a2 100644 --- a/src/console_completion.h +++ b/src/console_completion.h @@ -9,6 +9,9 @@ extern "C" { #endif +/* Current longest formatted match list is below this; overflow fails closed. */ +#define CONSOLE_COMPLETION_OUTPUT_CAPACITY 1024U + /* Install late-terminal upgrade handling and project-specific completion. */ void console_completion_install(void); diff --git a/src/serial_service.c b/src/serial_service.c index b06397d..4e12578 100644 --- a/src/serial_service.c +++ b/src/serial_service.c @@ -7,6 +7,7 @@ #include "driver/gpio.h" #include "driver/uart.h" #include "esp_check.h" +#include "esp_heap_caps.h" #include "esp_log.h" #include "freertos/queue.h" #include "freertos/semphr.h" @@ -17,8 +18,10 @@ #define SERIAL_UART_RX_RING_SIZE 8192 #define SERIAL_UART_EVENT_QUEUE_SIZE 64 -#define SERIAL_RX_STREAM_SIZE 16384 -#define SERIAL_TX_STREAM_SIZE 8192 +#define SERIAL_RX_STREAM_SIZE 16384U +#define SERIAL_TX_STREAM_SIZE 8192U +#define SERIAL_RX_STREAM_STORAGE_SIZE (SERIAL_RX_STREAM_SIZE + 1U) +#define SERIAL_TX_STREAM_STORAGE_SIZE (SERIAL_TX_STREAM_SIZE + 1U) #define SERIAL_IO_CHUNK_SIZE 256 #define SERIAL_TASK_STACK_SIZE 4096 #define SERIAL_TASK_PRIORITY 10 @@ -32,6 +35,10 @@ static SemaphoreHandle_t s_state_mutex; static SemaphoreHandle_t s_task_stopped; static StreamBufferHandle_t s_rx_stream; static StreamBufferHandle_t s_tx_stream; +static StaticStreamBuffer_t s_rx_stream_control; +static StaticStreamBuffer_t s_tx_stream_control; +static uint8_t *s_rx_stream_storage; +static uint8_t *s_tx_stream_storage; static QueueHandle_t s_uart_event_queue; static TaskHandle_t s_event_task; static portMUX_TYPE s_counter_lock = portMUX_INITIALIZER_UNLOCKED; @@ -472,8 +479,24 @@ esp_err_t serial_service_init(const serial_config_t *initial_config) s_state_mutex = xSemaphoreCreateMutex(); s_task_stopped = xSemaphoreCreateBinary(); - s_rx_stream = xStreamBufferCreate(SERIAL_RX_STREAM_SIZE, 1); - s_tx_stream = xStreamBufferCreate(SERIAL_TX_STREAM_SIZE, 1); + s_rx_stream_storage = heap_caps_calloc_prefer( + 1U, SERIAL_RX_STREAM_STORAGE_SIZE, 2, + MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, + MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + s_tx_stream_storage = heap_caps_calloc_prefer( + 1U, SERIAL_TX_STREAM_STORAGE_SIZE, 2, + MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, + MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + if (s_rx_stream_storage != NULL) { + s_rx_stream = xStreamBufferCreateStatic( + SERIAL_RX_STREAM_STORAGE_SIZE, 1U, s_rx_stream_storage, + &s_rx_stream_control); + } + if (s_tx_stream_storage != NULL) { + s_tx_stream = xStreamBufferCreateStatic( + SERIAL_TX_STREAM_STORAGE_SIZE, 1U, s_tx_stream_storage, + &s_tx_stream_control); + } if (s_state_mutex == NULL || s_task_stopped == NULL || s_rx_stream == NULL || s_tx_stream == NULL) { if (s_state_mutex != NULL) { @@ -488,10 +511,14 @@ esp_err_t serial_service_init(const serial_config_t *initial_config) if (s_tx_stream != NULL) { vStreamBufferDelete(s_tx_stream); } + heap_caps_free(s_rx_stream_storage); + heap_caps_free(s_tx_stream_storage); s_state_mutex = NULL; s_task_stopped = NULL; s_rx_stream = NULL; s_tx_stream = NULL; + s_rx_stream_storage = NULL; + s_tx_stream_storage = NULL; return ESP_ERR_NO_MEM; } diff --git a/src/ssh_transport.c b/src/ssh_transport.c index 238d7a2..c896949 100644 --- a/src/ssh_transport.c +++ b/src/ssh_transport.c @@ -89,6 +89,7 @@ static uint32_t s_external_close_id[SSH_TRANSPORT_MAX_SESSIONS]; static ssh_transport_counters_t s_counters; static SemaphoreHandle_t s_command_mutex; static bool s_initializing; +static bool s_init_faulted; static bool s_initialized; static bool s_running; static bool s_transitioning; @@ -1294,20 +1295,29 @@ esp_err_t ssh_transport_init(void) taskEXIT_CRITICAL(&s_lock); return ESP_OK; } - if (s_initializing) { + if (s_initializing || s_init_faulted) { taskEXIT_CRITICAL(&s_lock); return ESP_ERR_INVALID_STATE; } s_initializing = true; taskEXIT_CRITICAL(&s_lock); + bool wolfssh_initialized = false; esp_err_t error = secure_random_init(); if (error != ESP_OK) { error = ESP_ERR_INVALID_STATE; goto fail; } - if (wolfSSL_SetAllocators(ssh_malloc, ssh_free, ssh_realloc) != 0 || - wolfSSH_Init() != WS_SUCCESS || wc_SetSeed_Cb(ssh_seed) != 0) { + if (wolfSSL_SetAllocators(ssh_malloc, ssh_free, ssh_realloc) != 0) { + error = ESP_FAIL; + goto fail; + } + if (wolfSSH_Init() != WS_SUCCESS) { + error = ESP_FAIL; + goto fail; + } + wolfssh_initialized = true; + if (wc_SetSeed_Cb(ssh_seed) != 0) { error = ESP_FAIL; goto fail; } @@ -1347,7 +1357,9 @@ esp_err_t ssh_transport_init(void) return ESP_OK; fail: + bool cleanup_failed = wolfssh_initialized && wolfSSH_Cleanup() != WS_SUCCESS; taskENTER_CRITICAL(&s_lock); + s_init_faulted = cleanup_failed; s_initializing = false; taskEXIT_CRITICAL(&s_lock); return error; diff --git a/src/usb_cdc_transport.c b/src/usb_cdc_transport.c index 92fcf2c..818d57e 100644 --- a/src/usb_cdc_transport.c +++ b/src/usb_cdc_transport.c @@ -47,6 +47,7 @@ static portMUX_TYPE s_state_lock = portMUX_INITIALIZER_UNLOCKED; static atomic_bool s_initialized; static atomic_bool s_initializing; +static atomic_bool s_init_faulted; static atomic_uint s_usb_state; /* Changes on every effective CDC open/close boundary, even during one task poll. */ static atomic_uint s_connection_generation; @@ -816,13 +817,19 @@ static void reset_uninitialized_state(void) taskEXIT_CRITICAL(&s_state_lock); } -static void cleanup_init_allocations(bool cdc_initialized, bool driver_installed) +static esp_err_t cleanup_init_allocations(bool cdc_initialized, bool driver_installed) { if (cdc_initialized) { - (void)tinyusb_cdcacm_deinit(TINYUSB_CDC_ACM_0); + esp_err_t error = tinyusb_cdcacm_deinit(TINYUSB_CDC_ACM_0); + if (error != ESP_OK) { + return error; + } } if (driver_installed) { - (void)tinyusb_driver_uninstall(); + esp_err_t error = tinyusb_driver_uninstall(); + if (error != ESP_OK) { + return error; + } } if (s_control_queue != NULL) { vQueueDelete(s_control_queue); @@ -833,12 +840,27 @@ static void cleanup_init_allocations(bool cdc_initialized, bool driver_installed s_host_rx_stream = NULL; } reset_uninitialized_state(); + return ESP_OK; +} + +static esp_err_t fail_initialization(esp_err_t original_error, + bool cdc_initialized, + bool driver_installed) +{ + esp_err_t cleanup_error = cleanup_init_allocations(cdc_initialized, driver_installed); + if (cleanup_error != ESP_OK) { + /* Retain callback-facing storage and reject retries after incomplete teardown. */ + atomic_store(&s_init_faulted, true); + original_error = cleanup_error; + } + atomic_store(&s_initializing, false); + return original_error; } esp_err_t usb_cdc_transport_init(void) { bool expected = false; - if (atomic_load(&s_initialized) || + if (atomic_load(&s_initialized) || atomic_load(&s_init_faulted) || !atomic_compare_exchange_strong(&s_initializing, &expected, true)) { return ESP_ERR_INVALID_STATE; } @@ -870,9 +892,7 @@ esp_err_t usb_cdc_transport_init(void) s_control_queue = xQueueCreate(USB_CDC_CONTROL_QUEUE_LENGTH, sizeof(usb_cdc_control_t)); if (s_control_queue == NULL) { - cleanup_init_allocations(false, false); - atomic_store(&s_initializing, false); - return ESP_ERR_NO_MEM; + return fail_initialization(ESP_ERR_NO_MEM, false, false); } /* ESP32-S3's default full-speed internal PHY is fixed to GPIO19/20. */ @@ -884,9 +904,7 @@ esp_err_t usb_cdc_transport_init(void) result = tinyusb_driver_install(&usb_config); if (result != ESP_OK) { - cleanup_init_allocations(false, false); - atomic_store(&s_initializing, false); - return result; + return fail_initialization(result, false, false); } const tinyusb_config_cdcacm_t cdc_config = { @@ -898,9 +916,7 @@ esp_err_t usb_cdc_transport_init(void) }; result = tinyusb_cdcacm_init(&cdc_config); if (result != ESP_OK) { - cleanup_init_allocations(false, true); - atomic_store(&s_initializing, false); - return result; + return fail_initialization(result, false, true); } TaskHandle_t task = NULL; @@ -910,9 +926,7 @@ esp_err_t usb_cdc_transport_init(void) NULL, USB_CDC_TASK_PRIORITY, &task) != pdPASS) { - cleanup_init_allocations(true, true); - atomic_store(&s_initializing, false); - return ESP_ERR_NO_MEM; + return fail_initialization(ESP_ERR_NO_MEM, true, true); } atomic_store(&s_transport_task, (uintptr_t)task); diff --git a/src/user_database.c b/src/user_database.c index 4c88228..adfbe0b 100644 --- a/src/user_database.c +++ b/src/user_database.c @@ -6,6 +6,7 @@ #include #include +#include "esp_heap_caps.h" #include "freertos/FreeRTOS.h" #include "freertos/semphr.h" #include "mbedtls/ecp.h" @@ -66,7 +67,7 @@ typedef struct { static SemaphoreHandle_t s_mutex; static stored_database_t s_database; -static stored_database_t s_candidate; +static stored_database_t *s_candidate; static bool s_initialized; static uint8_t s_dummy_salt[USER_DATABASE_PASSWORD_SALT_LENGTH]; static uint8_t s_dummy_hash[USER_DATABASE_PASSWORD_HASH_LENGTH]; @@ -80,9 +81,30 @@ static esp_err_t initialize_dummy_verifier(void) return error; } +static esp_err_t allocate_candidate(void) +{ + if (s_candidate != NULL) { + return ESP_ERR_INVALID_STATE; + } + s_candidate = heap_caps_calloc_prefer( + 1U, sizeof(*s_candidate), 2, + MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT, + MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + return s_candidate != NULL ? ESP_OK : ESP_ERR_NO_MEM; +} + static void discard_candidate(void) { - secure_wipe(&s_candidate, sizeof(s_candidate)); + if (s_candidate != NULL) { + secure_wipe(s_candidate, sizeof(*s_candidate)); + } +} + +static void release_candidate(void) +{ + discard_candidate(); + heap_caps_free(s_candidate); + s_candidate = NULL; } static bool constant_time_equal(const uint8_t *left, const uint8_t *right, size_t length) @@ -437,10 +459,10 @@ static esp_err_t next_generation(uint32_t *generation) static esp_err_t commit_candidate_locked(void) { - recount(&s_candidate); - esp_err_t error = next_generation(&s_candidate.generation); + recount(s_candidate); + esp_err_t error = next_generation(&s_candidate->generation); if (error == ESP_OK) { - error = validate_database(&s_candidate); + error = validate_database(s_candidate); } if (error != ESP_OK) { discard_candidate(); @@ -451,7 +473,7 @@ static esp_err_t commit_candidate_locked(void) error = nvs_open(USER_DATABASE_NVS_NAMESPACE, NVS_READWRITE, &handle); if (error == ESP_OK) { error = nvs_set_blob(handle, USER_DATABASE_NVS_BLOB_KEY, - &s_candidate, sizeof(s_candidate)); + s_candidate, sizeof(*s_candidate)); if (error == ESP_OK) { error = nvs_commit(handle); } @@ -459,7 +481,7 @@ static esp_err_t commit_candidate_locked(void) } if (error == ESP_OK) { secure_wipe(&s_database, sizeof(s_database)); - s_database = s_candidate; + s_database = *s_candidate; } discard_candidate(); return error; @@ -530,8 +552,8 @@ static esp_err_t synchronize_legacy_locked( return error; } - s_candidate = s_database; - stored_user_t *candidate_user = &s_candidate.users[index]; + *s_candidate = s_database; + stored_user_t *candidate_user = &s_candidate->users[index]; error = set_record_password(candidate_user, legacy->password, legacy->password_length); if (error == ESP_OK) { @@ -557,12 +579,16 @@ esp_err_t user_database_init(const user_database_legacy_credentials_t *legacy, if (s_mutex == NULL) { return ESP_ERR_NO_MEM; } - esp_err_t error = nvs_flash_init(); + esp_err_t error = allocate_candidate(); if (error != ESP_OK) { vSemaphoreDelete(s_mutex); s_mutex = NULL; return error; } + error = nvs_flash_init(); + if (error != ESP_OK) { + goto init_failed; + } bool storage_missing = false; nvs_handle_t handle; @@ -621,7 +647,7 @@ esp_err_t user_database_init(const user_database_legacy_credentials_t *legacy, } *load_result = USER_DATABASE_LOAD_MIGRATED_LEGACY; recount(&s_database); - s_candidate = s_database; + *s_candidate = s_database; error = commit_candidate_locked(); if (error != ESP_OK) { goto init_failed; @@ -635,7 +661,7 @@ esp_err_t user_database_init(const user_database_legacy_credentials_t *legacy, init_failed: secure_wipe(&s_database, sizeof(s_database)); - discard_candidate(); + release_candidate(); secure_wipe(s_dummy_salt, sizeof(s_dummy_salt)); secure_wipe(s_dummy_hash, sizeof(s_dummy_hash)); vSemaphoreDelete(s_mutex); @@ -673,17 +699,23 @@ esp_err_t user_database_recover_from_legacy( if (s_mutex == NULL) { return ESP_ERR_NO_MEM; } - esp_err_t error = nvs_flash_init(); + esp_err_t error = allocate_candidate(); + if (error != ESP_OK) { + vSemaphoreDelete(s_mutex); + s_mutex = NULL; + return error; + } + error = nvs_flash_init(); if (error != ESP_OK) { goto recovery_failed; } memset(&s_database, 0, sizeof(s_database)); - memset(&s_candidate, 0, sizeof(s_candidate)); - s_candidate.version = USER_DATABASE_SCHEMA_VERSION; - s_candidate.size = sizeof(s_candidate); - s_candidate.generation = 1U; - error = initialize_user(&s_candidate.users[0], legacy->username, + memset(s_candidate, 0, sizeof(*s_candidate)); + s_candidate->version = USER_DATABASE_SCHEMA_VERSION; + s_candidate->size = sizeof(*s_candidate); + s_candidate->generation = 1U; + error = initialize_user(&s_candidate->users[0], legacy->username, legacy->username_length, USER_ROLE_USER, legacy->password, legacy->password_length); if (error == ESP_OK) { @@ -701,7 +733,7 @@ esp_err_t user_database_recover_from_legacy( recovery_failed: secure_wipe(&s_database, sizeof(s_database)); - discard_candidate(); + release_candidate(); secure_wipe(s_dummy_salt, sizeof(s_dummy_salt)); secure_wipe(s_dummy_hash, sizeof(s_dummy_hash)); vSemaphoreDelete(s_mutex); @@ -918,11 +950,11 @@ static esp_err_t create_locked(const uint8_t *username, size_t username_length, if (free_index < 0) { return ESP_ERR_NO_MEM; } - s_candidate = s_database; - esp_err_t error = initialize_user(&s_candidate.users[free_index], username, + *s_candidate = s_database; + esp_err_t error = initialize_user(&s_candidate->users[free_index], username, username_length, role, password, password_length); if (error == ESP_OK && role == USER_ROLE_ADMIN) { - s_candidate.admin_bootstrapped = 1U; + s_candidate->admin_bootstrapped = 1U; } if (error == ESP_OK) { return commit_candidate_locked(); @@ -984,15 +1016,15 @@ esp_err_t user_database_bootstrap_admin(const uint8_t *password, error = create_locked(s_admin_username, sizeof(s_admin_username) - 1U, USER_ROLE_ADMIN, password, password_length); } else { - s_candidate = s_database; - stored_user_t *user = &s_candidate.users[index]; + *s_candidate = s_database; + stored_user_t *user = &s_candidate->users[index]; error = set_record_password(user, password, password_length); if (error == ESP_OK) { user->role = USER_ROLE_ADMIN; error = next_generation(&user->auth_generation); } if (error == ESP_OK) { - s_candidate.admin_bootstrapped = 1U; + s_candidate->admin_bootstrapped = 1U; error = commit_candidate_locked(); } else { discard_candidate(); @@ -1026,7 +1058,7 @@ static esp_err_t mutate_user_begin(const uint8_t *username, size_t username_leng if (*index < 0) { return ESP_ERR_NOT_FOUND; } - s_candidate = s_database; + *s_candidate = s_database; return ESP_OK; } @@ -1050,7 +1082,7 @@ esp_err_t user_database_delete(const uint8_t *username, size_t username_length) error = ESP_ERR_INVALID_STATE; discard_candidate(); } else { - secure_wipe(&s_candidate.users[index], sizeof(s_candidate.users[index])); + secure_wipe(&s_candidate->users[index], sizeof(s_candidate->users[index])); error = commit_candidate_locked(); } } @@ -1069,7 +1101,7 @@ esp_err_t user_database_set_role(const uint8_t *username, size_t username_length int index; esp_err_t error = mutate_user_begin(username, username_length, &index); if (error == ESP_OK) { - stored_user_t *user = &s_candidate.users[index]; + stored_user_t *user = &s_candidate->users[index]; if (user->role == role) { error = ESP_OK; discard_candidate(); @@ -1080,7 +1112,7 @@ esp_err_t user_database_set_role(const uint8_t *username, size_t username_length user->role = (uint8_t)role; error = next_generation(&user->auth_generation); if (error == ESP_OK && role == USER_ROLE_ADMIN) { - s_candidate.admin_bootstrapped = 1U; + s_candidate->admin_bootstrapped = 1U; } if (error == ESP_OK) { error = commit_candidate_locked(); @@ -1104,7 +1136,7 @@ esp_err_t user_database_set_password(const uint8_t *username, size_t username_le int index; esp_err_t error = mutate_user_begin(username, username_length, &index); if (error == ESP_OK) { - stored_user_t *user = &s_candidate.users[index]; + stored_user_t *user = &s_candidate->users[index]; error = set_record_password(user, password, password_length); if (error == ESP_OK) { error = next_generation(&user->auth_generation); @@ -1149,7 +1181,7 @@ esp_err_t user_database_add_ssh_key( int user_index; esp_err_t error = mutate_user_begin(username, username_length, &user_index); if (error == ESP_OK) { - stored_user_t *user = &s_candidate.users[user_index]; + stored_user_t *user = &s_candidate->users[user_index]; int free_index = -1; for (size_t index = 0U; index < USER_DATABASE_MAX_SSH_KEYS_PER_USER; ++index) { @@ -1210,7 +1242,7 @@ esp_err_t user_database_remove_ssh_key(const uint8_t *username, int user_index; esp_err_t error = mutate_user_begin(username, username_length, &user_index); if (error == ESP_OK) { - stored_user_t *user = &s_candidate.users[user_index]; + stored_user_t *user = &s_candidate->users[user_index]; if (user->keys[key_index].active == 0U) { error = ESP_ERR_NOT_FOUND; discard_candidate(); @@ -1239,7 +1271,7 @@ esp_err_t user_database_clear_ssh_keys(const uint8_t *username, int user_index; esp_err_t error = mutate_user_begin(username, username_length, &user_index); if (error == ESP_OK) { - stored_user_t *user = &s_candidate.users[user_index]; + stored_user_t *user = &s_candidate->users[user_index]; if (user->key_count == 0U) { error = ESP_OK; discard_candidate(); diff --git a/src/web_server.c b/src/web_server.c index 87853e6..7069aa9 100644 --- a/src/web_server.c +++ b/src/web_server.c @@ -695,8 +695,13 @@ esp_err_t web_server_start(void) serial_transport_attached = attach_error == ESP_OK; } if (error != ESP_OK && server != NULL) { - (void)httpd_ssl_stop(server); - server = NULL; + esp_err_t cleanup_error = httpd_ssl_stop(server); + if (cleanup_error == ESP_OK) { + server = NULL; + } else { + /* Retain ownership so stop can retry and start cannot allocate a second server. */ + error = cleanup_error; + } } xSemaphoreTake(s_server_mutex, portMAX_DELAY); @@ -708,6 +713,8 @@ esp_err_t web_server_start(void) s_server = server; ++s_counters.starts; } else { + /* A non-NULL handle is a partially started server whose cleanup failed. */ + s_server = server; ++s_counters.start_failures; } xSemaphoreGive(s_server_mutex);