Merge branch 'fix/esptimer_dump_v6.1' into 'release/v6.1'

fix(esp_timer): possible heap overflow on info dump (v6.1)

See merge request espressif/esp-idf!50996

components/esp_timer/src/esp_timer.c

@@ -5,6 +5,7 @@
  */
 
 #include <sys/param.h>
+#include <stdarg.h>
 #include <string.h>
 #include "soc/soc.h"
 #include "esp_types.h"
@@ -695,28 +696,54 @@ esp_err_t esp_timer_deinit(void)
     return ESP_OK;
 }
 
+static void append_to_buffer(char** dst, size_t* dst_size, const char* format, ...)
+{
+    /* Defensive: avoid underflow in the truncation path below if no space remains. */
+    if (*dst_size == 0) {
+        return;
+    }
+
+    va_list args;
+    va_start(args, format);
+    int cb = vsnprintf(*dst, *dst_size, format, args);
+    va_end(args);
+
+    if (cb < 0) {
+        return;
+    }
+
+    /* On truncation, snprintf returns the full would-be length. Keep the
+     * cursor inside the buffer and preserve the terminating NUL byte.
+     */
+    if ((size_t) cb >= *dst_size) {
+        *dst += *dst_size - 1;
+        *dst_size = 1;
+        return;
+    }
+
+    *dst += cb;
+    *dst_size -= cb;
+}
+
 static void print_timer_info(esp_timer_handle_t t, char** dst, size_t* dst_size)
 {
 #if WITH_PROFILING
-    size_t cb;
     // name is optional, might be missed.
     if (t->name) {
-        cb = snprintf(*dst, *dst_size, "%-20.20s  ", t->name);
+        append_to_buffer(dst, dst_size, "%-20.20s  ", t->name);
     } else {
-        cb = snprintf(*dst, *dst_size, "timer@%-10p  ", t);
+        append_to_buffer(dst, dst_size, "timer@%-10p  ", t);
     }
 
-    cb += snprintf(*dst + cb, *dst_size - cb, "%-10lld  %-12lld  %-12d  %-12d  %-12d  %-12lld\n",
-                   (uint64_t)t->period, t->alarm, t->times_armed,
-                   t->times_triggered, t->times_skipped, t->total_callback_run_time);
+    append_to_buffer(dst, dst_size, "%-10lld  %-12lld  %-12d  %-12d  %-12d  %-12lld\n",
+                     (uint64_t)t->period, t->alarm, t->times_armed,
+                     t->times_triggered, t->times_skipped, t->total_callback_run_time);
     /* keep this in sync with the format string, used in esp_timer_dump */
 #define TIMER_INFO_LINE_LEN 103
 #else
-    size_t cb = snprintf(*dst, *dst_size, "timer@%-14p  %-10lld  %-12lld\n", t, (uint64_t)t->period, t->alarm);
+    append_to_buffer(dst, dst_size, "timer@%-14p  %-10lld  %-12lld\n", t, (uint64_t)t->period, t->alarm);
 #define TIMER_INFO_LINE_LEN 47
 #endif
-    *dst += cb;
-    *dst_size -= cb;
 }
 
 esp_err_t esp_timer_dump(FILE* stream)
@@ -750,6 +777,7 @@ esp_err_t esp_timer_dump(FILE* stream)
      * slightly more and the output will be truncated if that is not enough.
      */
     size_t buf_size = TIMER_INFO_LINE_LEN * (timer_count + 3);
+    /* buf_size is the snprintf size, including NUL; keep one extra byte as slack. */
     char* print_buf = calloc(1, buf_size + 1);
     if (print_buf == NULL) {
         return ESP_ERR_NO_MEM;

components/esp_timer/test_apps/main/CMakeLists.txt

@@ -23,3 +23,12 @@ idf_component_register(SRCS ${srcs}
                        PRIV_INCLUDE_DIRS "../../private_include" "../include"
                        PRIV_REQUIRES cmock test_utils esp_timer spi_flash esp_psram esp_driver_gpio esp_pm
                        WHOLE_ARCHIVE)
+
+if(CONFIG_IDF_TARGET_LINUX AND NOT CMAKE_HOST_SYSTEM_NAME STREQUAL "Darwin")
+    # Linux host heap integrity checks are stubs, so use ASan to catch dump overflows.
+    set(asan_options -fsanitize=address -fno-omit-frame-pointer)
+    idf_component_get_property(esp_timer_lib esp_timer COMPONENT_LIB)
+    target_compile_options(${esp_timer_lib} PRIVATE ${asan_options})
+    target_compile_options(${COMPONENT_LIB} PRIVATE ${asan_options})
+    idf_build_set_property(LINK_OPTIONS -fsanitize=address APPEND)
+endif()

components/esp_timer/test_apps/main/test_esp_timer.c

@@ -6,6 +6,7 @@
 #include "sdkconfig.h"
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <inttypes.h>
 #include <time.h>
 #include <sys/time.h>
@@ -506,25 +507,57 @@ TEST_CASE("Can dump esp_timer stats", "[esp_timer]")
        overflow the internal string buffer if the
        length calculation is not correct.
     */
-    const int NUM_TIMERS = 200;
-    esp_timer_handle_t timers[NUM_TIMERS];
+    enum {
+        INACTIVE_TIMER_COUNT = 64,
+        NUM_TIMERS = 256,
+    };
+    const uint64_t long_period = (1ULL << 56) - 1;
+    const uint64_t long_alarm = (uint64_t)esp_timer_get_time() + 1000000000000ULL;
+    /* Keep large test buffers off the Unity task stack on smaller targets. */
+    static esp_timer_handle_t inactive_timers[INACTIVE_TIMER_COUNT];
+    static char inactive_names[INACTIVE_TIMER_COUNT][30];
+    static esp_timer_handle_t timers[NUM_TIMERS];
 
-    for (int i = 0; i < NUM_TIMERS; ++i) {
-        char name[30];
-        snprintf(name, sizeof(name), "test_timer_number_%d", i);
-        esp_timer_create_args_t timer_args = {
+    for (size_t i = 0; i < INACTIVE_TIMER_COUNT; ++i) {
+        snprintf(inactive_names[i], sizeof(inactive_names[i]), "test_timer_number_%zu", i);
+        const esp_timer_create_args_t timer_args = {
             .callback = &empty_cb,
             .arg = NULL,
-            .name = name
+            .name = inactive_names[i],
         };
-        TEST_ESP_OK(esp_timer_create(&timer_args, &timers[i]));
+        TEST_ESP_OK(esp_timer_create(&timer_args, &inactive_timers[i]));
     }
 
-    esp_timer_dump(stdout);
+    for (size_t i = 0; i < NUM_TIMERS; ++i) {
+        const esp_timer_create_args_t timer_args = {
+            .callback = &empty_cb,
+            .name = "dump_long_line_timer",
+        };
+        TEST_ESP_OK(esp_timer_create(&timer_args, &timers[i]));
+        TEST_ESP_OK(esp_timer_start_periodic_at(timers[i], long_period, long_alarm + i));
+    }
 
-    for (int i = 0; i < NUM_TIMERS; ++i) {
+    static char dump_buf[4096];
+    memset(dump_buf, 0, sizeof(dump_buf));
+    FILE* stream = fmemopen(dump_buf, sizeof(dump_buf) - 1, "w+");
+    TEST_ASSERT_NOT_NULL(stream);
+    TEST_ESP_OK(esp_timer_dump(stream));
+    fflush(stream);
+    fclose(stream);
+    TEST_ASSERT_NOT_NULL(strstr(dump_buf, "Timer stats:"));
+    TEST_ASSERT_NOT_NULL(strstr(dump_buf, "72057594037927935"));
+#if !CONFIG_IDF_TARGET_LINUX
+    TEST_ASSERT_TRUE(heap_caps_check_integrity_all(true));
+#endif
+
+    for (size_t i = 0; i < NUM_TIMERS; ++i) {
+        TEST_ESP_OK(esp_timer_stop(timers[i]));
         TEST_ESP_OK(esp_timer_delete(timers[i]));
     }
+
+    for (size_t i = 0; i < INACTIVE_TIMER_COUNT; ++i) {
+        TEST_ESP_OK(esp_timer_delete(inactive_timers[i]));
+    }
 }
 
 typedef struct {