Merge branch 'fix/fix_esp_local_ctrl_potential_double_free' into 'master'

fix(esp_local_ctrl): fixes a potential double free

See merge request espressif/esp-idf!46953

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;
 }

components/esp_local_ctrl/src/esp_local_ctrl_transport_ble.c

@@ -1,24 +1,16 @@
-// 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 <stdio.h>
-#include <string.h>
 #include <esp_err.h>
 #include <esp_log.h>
+#include <stdio.h>
+#include <string.h>
 
-#include <protocomm_ble.h>
 #include <esp_local_ctrl.h>
+#include <protocomm_ble.h>
 
 #include "esp_local_ctrl_priv.h"
 
@@ -26,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");
@@ -40,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;
@@ -54,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;
@@ -77,29 +72,42 @@ static esp_err_t copy_ble_config(esp_local_ctrl_transport_config_t *dest_config,
             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);
+            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)
+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;
@@ -119,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);
@@ -131,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;
 };

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

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)

components/esp_local_ctrl/test_apps/README.md

@@ -0,0 +1,2 @@
+| Supported Targets | ESP32-C3 |
+| ----------------- | -------- |

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
+)

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();
+}

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 <string.h>
+#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());
+}

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

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