From 544e71e2635a62c83b00514135fc35b771af53d7 Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Tue, 24 Mar 2026 14:45:05 +0800 Subject: [PATCH 1/2] fix(esp_local_ctrl): fixes a potential double free --- .../esp_local_ctrl/src/esp_local_ctrl.c | 4 +++ .../src/esp_local_ctrl_transport_ble.c | 34 +++++++++++-------- tools/ci/check_copyright_ignore.txt | 1 - 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/components/esp_local_ctrl/src/esp_local_ctrl.c b/components/esp_local_ctrl/src/esp_local_ctrl.c index 77f19ec118..3b286bc427 100644 --- a/components/esp_local_ctrl/src/esp_local_ctrl.c +++ b/components/esp_local_ctrl/src/esp_local_ctrl.c @@ -330,6 +330,10 @@ esp_err_t esp_local_ctrl_remove_property(const char *name) } local_ctrl_inst_ctx->props[i-1] = local_ctrl_inst_ctx->props[i]; } + /* Clear the stale pointer left in the last slot after compaction to + * prevent a double-free if esp_local_ctrl_stop() is called before the + * slot is overwritten by a subsequent add_property(). */ + local_ctrl_inst_ctx->props[local_ctrl_inst_ctx->props_count - 1] = NULL; local_ctrl_inst_ctx->props_count--; return ESP_OK; } diff --git a/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c b/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c index 248b410899..3f77f6caa5 100644 --- a/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c +++ b/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c @@ -1,16 +1,8 @@ -// Copyright 2019 Espressif Systems (Shanghai) PTE LTD -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. +/* + * SPDX-FileCopyrightText: 2019-2026 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ #include #include @@ -73,21 +65,33 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, free(dest_config->ble); return ESP_ERR_NO_MEM; } + esp_err_t ret = ESP_OK; for (uint16_t i = 0; i < src_config->ble->nu_lookup_count; i++) { dest_config->ble->nu_lookup[i].uuid = src_config->ble->nu_lookup[i].uuid; if (!src_config->ble->nu_lookup[i].name) { ESP_LOGE(TAG, "Endpoint name cannot be null"); - return ESP_ERR_INVALID_ARG; + ret = ESP_ERR_INVALID_ARG; + goto err_free_nu_lookup; } dest_config->ble->nu_lookup[i].name = strdup(src_config->ble->nu_lookup[i].name); if (!dest_config->ble->nu_lookup[i].name) { ESP_LOGE(TAG, "Failed to allocate memory for endpoint name"); - return ESP_ERR_NO_MEM; + ret = ESP_ERR_NO_MEM; + goto err_free_nu_lookup; } dest_config->ble->nu_lookup_count++; } } return ESP_OK; + +err_free_nu_lookup: + for (uint16_t i = 0; i < dest_config->ble->nu_lookup_count; i++) { + free((void *) dest_config->ble->nu_lookup[i].name); + } + free(dest_config->ble->nu_lookup); + free(dest_config->ble); + dest_config->ble = NULL; + return ret; } static esp_err_t declare_endpoint(esp_local_ctrl_transport_config_t *config, const char *ep_name, uint16_t ep_uuid) diff --git a/tools/ci/check_copyright_ignore.txt b/tools/ci/check_copyright_ignore.txt index 0a7316fe69..bbcf995f91 100644 --- a/tools/ci/check_copyright_ignore.txt +++ b/tools/ci/check_copyright_ignore.txt @@ -378,7 +378,6 @@ components/esp_hid/include/esp_hidh_gattc.h components/esp_hid/private/bt_hidd.h components/esp_hid/private/bt_hidh.h components/esp_local_ctrl/src/esp_local_ctrl_priv.h -components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c components/esp_rom/esp32/include/esp32/rom/tjpgd.h components/esp_rom/esp32/ld/esp32.rom.api.ld components/esp_rom/esp32/ld/esp32.rom.eco3.ld From 1ebb75e5981c1cf23b03aabed0cd683b1d2ccf5f Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Tue, 24 Mar 2026 15:04:46 +0800 Subject: [PATCH 2/2] feat(esp_local_ctrl): adds basic test app --- .../src/esp_local_ctrl_transport_ble.c | 46 +++--- .../test_apps/.build-test-rules.yml | 8 + .../esp_local_ctrl/test_apps/CMakeLists.txt | 4 + components/esp_local_ctrl/test_apps/README.md | 2 + .../test_apps/main/CMakeLists.txt | 8 + .../esp_local_ctrl/test_apps/main/app_main.c | 45 ++++++ .../test_apps/main/test_esp_local_ctrl.c | 139 ++++++++++++++++++ .../test_apps/sdkconfig.defaults | 4 + 8 files changed, 235 insertions(+), 21 deletions(-) create mode 100644 components/esp_local_ctrl/test_apps/.build-test-rules.yml create mode 100644 components/esp_local_ctrl/test_apps/CMakeLists.txt create mode 100644 components/esp_local_ctrl/test_apps/README.md create mode 100644 components/esp_local_ctrl/test_apps/main/CMakeLists.txt create mode 100644 components/esp_local_ctrl/test_apps/main/app_main.c create mode 100644 components/esp_local_ctrl/test_apps/main/test_esp_local_ctrl.c create mode 100644 components/esp_local_ctrl/test_apps/sdkconfig.defaults diff --git a/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c b/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c index 3f77f6caa5..f0954edf14 100644 --- a/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c +++ b/components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c @@ -4,13 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -#include -#include #include #include +#include +#include -#include #include +#include #include "esp_local_ctrl_priv.h" @@ -18,7 +18,9 @@ static const char *TAG = "esp_local_ctrl_transport_ble"; -static esp_err_t start_ble_transport(protocomm_t *pc, const esp_local_ctrl_transport_config_t *config) +static esp_err_t +start_ble_transport(protocomm_t *pc, + const esp_local_ctrl_transport_config_t *config) { if (!config || !config->ble) { ESP_LOGE(TAG, "NULL configuration provided"); @@ -32,8 +34,11 @@ static void stop_ble_transport(protocomm_t *pc) protocomm_ble_stop(pc); } -static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, const esp_local_ctrl_transport_config_t *src_config) +static esp_err_t +copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, + const esp_local_ctrl_transport_config_t *src_config) { + esp_err_t ret = ESP_OK; if (!dest_config || !src_config || !src_config->ble) { ESP_LOGE(TAG, "NULL arguments provided"); return ESP_ERR_INVALID_ARG; @@ -46,13 +51,11 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, } /* Copy BLE device name */ - memcpy(dest_config->ble->device_name, - src_config->ble->device_name, + memcpy(dest_config->ble->device_name, src_config->ble->device_name, sizeof(src_config->ble->device_name)); /* Copy Service UUID */ - memcpy(dest_config->ble->service_uuid, - src_config->ble->service_uuid, + memcpy(dest_config->ble->service_uuid, src_config->ble->service_uuid, sizeof(src_config->ble->service_uuid)); dest_config->ble->nu_lookup_count = 0; @@ -65,7 +68,6 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, free(dest_config->ble); return ESP_ERR_NO_MEM; } - esp_err_t ret = ESP_OK; for (uint16_t i = 0; i < src_config->ble->nu_lookup_count; i++) { dest_config->ble->nu_lookup[i].uuid = src_config->ble->nu_lookup[i].uuid; if (!src_config->ble->nu_lookup[i].name) { @@ -73,7 +75,8 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, ret = ESP_ERR_INVALID_ARG; goto err_free_nu_lookup; } - dest_config->ble->nu_lookup[i].name = strdup(src_config->ble->nu_lookup[i].name); + dest_config->ble->nu_lookup[i].name = + strdup(src_config->ble->nu_lookup[i].name); if (!dest_config->ble->nu_lookup[i].name) { ESP_LOGE(TAG, "Failed to allocate memory for endpoint name"); ret = ESP_ERR_NO_MEM; @@ -86,7 +89,7 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config, err_free_nu_lookup: for (uint16_t i = 0; i < dest_config->ble->nu_lookup_count; i++) { - free((void *) dest_config->ble->nu_lookup[i].name); + free((void *)dest_config->ble->nu_lookup[i].name); } free(dest_config->ble->nu_lookup); free(dest_config->ble); @@ -94,16 +97,17 @@ err_free_nu_lookup: return ret; } -static esp_err_t declare_endpoint(esp_local_ctrl_transport_config_t *config, const char *ep_name, uint16_t ep_uuid) +static esp_err_t declare_endpoint(esp_local_ctrl_transport_config_t *config, + const char *ep_name, uint16_t ep_uuid) { if (!config || !config->ble) { ESP_LOGE(TAG, "NULL configuration provided"); return ESP_ERR_INVALID_ARG; } - protocomm_ble_name_uuid_t *nu_lookup = realloc(config->ble->nu_lookup, - (config->ble->nu_lookup_count + 1) - * sizeof(protocomm_ble_name_uuid_t)); + protocomm_ble_name_uuid_t *nu_lookup = + realloc(config->ble->nu_lookup, (config->ble->nu_lookup_count + 1) * + sizeof(protocomm_ble_name_uuid_t)); if (!nu_lookup) { ESP_LOGE(TAG, "Failed to allocate memory for new endpoint entry"); return ESP_ERR_NO_MEM; @@ -123,7 +127,7 @@ static void free_config(esp_local_ctrl_transport_config_t *config) { if (config && config->ble) { for (unsigned int i = 0; i < config->ble->nu_lookup_count; i++) { - free((void*) config->ble->nu_lookup[i].name); + free((void *)config->ble->nu_lookup[i].name); } free(config->ble->nu_lookup); free(config->ble); @@ -135,10 +139,10 @@ const esp_local_ctrl_transport_t *esp_local_ctrl_get_transport_ble(void) { static const esp_local_ctrl_transport_t tp = { .start_service = start_ble_transport, - .stop_service = stop_ble_transport, - .copy_config = copy_ble_config, - .declare_ep = declare_endpoint, - .free_config = free_config + .stop_service = stop_ble_transport, + .copy_config = copy_ble_config, + .declare_ep = declare_endpoint, + .free_config = free_config }; return &tp; }; diff --git a/components/esp_local_ctrl/test_apps/.build-test-rules.yml b/components/esp_local_ctrl/test_apps/.build-test-rules.yml new file mode 100644 index 0000000000..f05b9ceee0 --- /dev/null +++ b/components/esp_local_ctrl/test_apps/.build-test-rules.yml @@ -0,0 +1,8 @@ +# Documentation: .gitlab/ci/README.md#manifest-file-to-control-the-buildtest-apps + +components/esp_local_ctrl/test_apps: + disable: + - if: IDF_TARGET not in ["esp32c3"] + reason: Testing on one target is enough + depends_components: + - esp_local_ctrl diff --git a/components/esp_local_ctrl/test_apps/CMakeLists.txt b/components/esp_local_ctrl/test_apps/CMakeLists.txt new file mode 100644 index 0000000000..86cc11ddc5 --- /dev/null +++ b/components/esp_local_ctrl/test_apps/CMakeLists.txt @@ -0,0 +1,4 @@ +cmake_minimum_required(VERSION 3.22) +set(COMPONENTS main) +include($ENV{IDF_PATH}/tools/cmake/project.cmake) +project(esp_local_ctrl_test) diff --git a/components/esp_local_ctrl/test_apps/README.md b/components/esp_local_ctrl/test_apps/README.md new file mode 100644 index 0000000000..67844e9673 --- /dev/null +++ b/components/esp_local_ctrl/test_apps/README.md @@ -0,0 +1,2 @@ +| Supported Targets | ESP32-C3 | +| ----------------- | -------- | diff --git a/components/esp_local_ctrl/test_apps/main/CMakeLists.txt b/components/esp_local_ctrl/test_apps/main/CMakeLists.txt new file mode 100644 index 0000000000..788ba068e6 --- /dev/null +++ b/components/esp_local_ctrl/test_apps/main/CMakeLists.txt @@ -0,0 +1,8 @@ +idf_component_register( + SRCS "app_main.c" "test_esp_local_ctrl.c" + # Access the private transport struct so we can define a null transport + # without requiring BLE or HTTPD hardware in unit tests. + PRIV_INCLUDE_DIRS "$ENV{IDF_PATH}/components/esp_local_ctrl/src" + PRIV_REQUIRES unity esp_local_ctrl + WHOLE_ARCHIVE +) diff --git a/components/esp_local_ctrl/test_apps/main/app_main.c b/components/esp_local_ctrl/test_apps/main/app_main.c new file mode 100644 index 0000000000..2d8e71939c --- /dev/null +++ b/components/esp_local_ctrl/test_apps/main/app_main.c @@ -0,0 +1,45 @@ +/* + * SPDX-FileCopyrightText: 2026 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ +#include "unity.h" +#include "unity_test_runner.h" +#include "esp_heap_caps.h" + +#define TEST_MEMORY_LEAK_THRESHOLD (-512) + +static size_t s_before_free_8bit; +static size_t s_before_free_32bit; + +static void check_leak(size_t before_free, size_t after_free, const char *type) +{ + ssize_t delta = after_free - before_free; + printf("MALLOC_CAP_%s: Before %u bytes free, After %u bytes free (delta %d)\n", + type, before_free, after_free, delta); + TEST_ASSERT_MESSAGE(delta >= TEST_MEMORY_LEAK_THRESHOLD, "memory leak"); +} + +void setUp(void) +{ + s_before_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); + s_before_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); +} + +void tearDown(void) +{ + /* Catch heap corruption that would be caused by a double-free */ + TEST_ASSERT_MESSAGE(heap_caps_check_integrity(MALLOC_CAP_INVALID, true), + "heap integrity check failed (possible double-free or corruption)"); + + size_t after_free_8bit = heap_caps_get_free_size(MALLOC_CAP_8BIT); + size_t after_free_32bit = heap_caps_get_free_size(MALLOC_CAP_32BIT); + check_leak(s_before_free_8bit, after_free_8bit, "8BIT"); + check_leak(s_before_free_32bit, after_free_32bit, "32BIT"); +} + +void app_main(void) +{ + printf("Running esp_local_ctrl component tests\n"); + unity_run_menu(); +} diff --git a/components/esp_local_ctrl/test_apps/main/test_esp_local_ctrl.c b/components/esp_local_ctrl/test_apps/main/test_esp_local_ctrl.c new file mode 100644 index 0000000000..9cc61cf90a --- /dev/null +++ b/components/esp_local_ctrl/test_apps/main/test_esp_local_ctrl.c @@ -0,0 +1,139 @@ +/* + * SPDX-FileCopyrightText: 2026 Espressif Systems (Shanghai) CO LTD + * + * SPDX-License-Identifier: Apache-2.0 + */ +#include +#include "unity.h" +#include "esp_local_ctrl.h" +#include "esp_local_ctrl_priv.h" /* full definition of esp_local_ctrl_transport */ +#include "esp_heap_caps.h" + +/* + * Minimal transport with all function pointers NULL. + * + * esp_local_ctrl_start() guards every transport callback with an `if` before + * calling it, so a zero-initialised transport lets the service start with an + * in-memory protocomm instance only — no BLE or HTTPD hardware required. + */ +static const esp_local_ctrl_transport_t s_null_transport = { 0 }; + +static esp_err_t dummy_get_prop_values(size_t props_count, + const esp_local_ctrl_prop_t props[], + esp_local_ctrl_prop_val_t prop_values[], + void *usr_ctx) +{ + return ESP_OK; +} + +static esp_err_t dummy_set_prop_values(size_t props_count, + const esp_local_ctrl_prop_t props[], + const esp_local_ctrl_prop_val_t prop_values[], + void *usr_ctx) +{ + return ESP_OK; +} + +static esp_local_ctrl_config_t make_config(size_t max_properties) +{ + esp_local_ctrl_config_t cfg = { + .transport = &s_null_transport, + .transport_config = { .httpd = NULL }, + .proto_sec = { .version = PROTOCOM_SEC0 }, + .handlers = { + .get_prop_values = dummy_get_prop_values, + .set_prop_values = dummy_set_prop_values, + }, + .max_properties = max_properties, + }; + return cfg; +} + +/* + * Test: remove a non-last property, then stop. + * + * 1. Add prop_a (slot 0) and prop_b (slot 1). props_count = 2. + * 2. Remove prop_a: + * - prop_a is freed. + * - Compaction shifts prop_b into slot 0. + * 3. esp_local_ctrl_stop() must free prop_b exactly once (slot 0). + */ +TEST_CASE("remove_property then stop does not double-free", "[esp_local_ctrl]") +{ + esp_local_ctrl_config_t cfg = make_config(5); + TEST_ESP_OK(esp_local_ctrl_start(&cfg)); + + const esp_local_ctrl_prop_t prop_a = { .name = "prop_a" }; + const esp_local_ctrl_prop_t prop_b = { .name = "prop_b" }; + TEST_ESP_OK(esp_local_ctrl_add_property(&prop_a)); + TEST_ESP_OK(esp_local_ctrl_add_property(&prop_b)); + + /* Removing the first property triggers compaction. */ + TEST_ESP_OK(esp_local_ctrl_remove_property("prop_a")); + + /* prop_b must still be reachable after compaction */ + TEST_ASSERT_NOT_NULL(esp_local_ctrl_get_property("prop_b")); + + TEST_ESP_OK(esp_local_ctrl_stop()); +} + +/* Test: remove the only property then stop — no compaction, basic sanity. */ +TEST_CASE("remove sole property then stop is clean", "[esp_local_ctrl]") +{ + esp_local_ctrl_config_t cfg = make_config(3); + TEST_ESP_OK(esp_local_ctrl_start(&cfg)); + + const esp_local_ctrl_prop_t prop = { .name = "only_prop" }; + TEST_ESP_OK(esp_local_ctrl_add_property(&prop)); + TEST_ESP_OK(esp_local_ctrl_remove_property("only_prop")); + + /* After removing the sole property, stop must find nothing to free. */ + TEST_ESP_OK(esp_local_ctrl_stop()); +} + +/* Test: multiple compaction passes (remove head repeatedly) then stop. */ +TEST_CASE("repeated head removal then stop is clean", "[esp_local_ctrl]") +{ + esp_local_ctrl_config_t cfg = make_config(5); + TEST_ESP_OK(esp_local_ctrl_start(&cfg)); + + const esp_local_ctrl_prop_t p0 = { .name = "p0" }; + const esp_local_ctrl_prop_t p1 = { .name = "p1" }; + const esp_local_ctrl_prop_t p2 = { .name = "p2" }; + TEST_ESP_OK(esp_local_ctrl_add_property(&p0)); + TEST_ESP_OK(esp_local_ctrl_add_property(&p1)); + TEST_ESP_OK(esp_local_ctrl_add_property(&p2)); + + /* First removal: p1→slot0, p2→slot1, slot2 stale (fixed to NULL). */ + TEST_ESP_OK(esp_local_ctrl_remove_property("p0")); + /* Second removal: p2→slot0, slot1 stale (fixed to NULL). */ + TEST_ESP_OK(esp_local_ctrl_remove_property("p1")); + + TEST_ASSERT_NOT_NULL(esp_local_ctrl_get_property("p2")); + TEST_ESP_OK(esp_local_ctrl_stop()); +} + +/* Test: add/remove/re-add cycle — the slot vacated by a remove must accept a + * new property without corrupting internal bookkeeping. */ +TEST_CASE("add remove add cycle is clean", "[esp_local_ctrl]") +{ + esp_local_ctrl_config_t cfg = make_config(3); + TEST_ESP_OK(esp_local_ctrl_start(&cfg)); + + const esp_local_ctrl_prop_t pa = { .name = "a" }; + const esp_local_ctrl_prop_t pb = { .name = "b" }; + const esp_local_ctrl_prop_t pc = { .name = "c" }; + + TEST_ESP_OK(esp_local_ctrl_add_property(&pa)); + TEST_ESP_OK(esp_local_ctrl_add_property(&pb)); + TEST_ESP_OK(esp_local_ctrl_remove_property("a")); + + /* Reuse the freed slot capacity for a new property. */ + TEST_ESP_OK(esp_local_ctrl_add_property(&pc)); + + TEST_ASSERT_NULL(esp_local_ctrl_get_property("a")); + TEST_ASSERT_NOT_NULL(esp_local_ctrl_get_property("b")); + TEST_ASSERT_NOT_NULL(esp_local_ctrl_get_property("c")); + + TEST_ESP_OK(esp_local_ctrl_stop()); +} diff --git a/components/esp_local_ctrl/test_apps/sdkconfig.defaults b/components/esp_local_ctrl/test_apps/sdkconfig.defaults new file mode 100644 index 0000000000..33c4635072 --- /dev/null +++ b/components/esp_local_ctrl/test_apps/sdkconfig.defaults @@ -0,0 +1,4 @@ +# Enable Security Version 0 (no encryption) so esp_local_ctrl_start() succeeds +# with a minimal null transport (no BLE/Wi-Fi hardware required). +CONFIG_ESP_PROTOCOMM_SUPPORT_SECURITY_VERSION_0=y +CONFIG_ESP_TASK_WDT_INIT=n