core/thread: fix thread_measure_stack_free()

`thread_measure_stack_free()` previously assumed that reading past the stack is safe. When the stack was indeed part of a thread, the `thread_t` structure is put after the stack, increasing the odds of this assumption to hold. However, `thread_measure_stack_free()` could also be used on the ISR stack, which may be allocated at the end of SRAM. A second parameter had to be added to indicate the stack size, so that reading past the stack can now be prevented. This also makes valgrind happy on `native`/`native64`.
RIOT-OS · maribu · May 31, 2024 · Nov 21, 2022 · May 31, 2024 · May 31, 2024
commit e93b5e4b98a8bb6254dbab5047669dd02a4f2ef0
@@ -450,15 +450,20 @@ static inline const char *thread_getname(kernel_pid_t pid)
 #endif
 
 /**
- * @brief Measures the stack usage of a stack
+ * @brief       Measures the stack usage of a stack
+ * @internal    Should not be used externally
  *
- * Only works if the thread was created with the flag THREAD_CREATE_STACKTEST.
+ * Only works if the stack is filled with canaries
+ * (`*((uintptr_t *)ptr) == (uintptr_t)ptr` for naturally aligned `ptr` within
+ * the stack).
  *
- * @param[in] stack the stack you want to measure. Try `thread_get_stackstart(thread_get_active())`
+ * @param[in] stack     the stack you want to measure. Try
+ *                      `thread_get_stackstart(thread_get_active())`
+ * @param[in] size      size of @p stack in bytes
  *
- * @return          the amount of unused space of the thread's stack
+ * @return              the amount of unused space of the thread's stack
  */
-uintptr_t thread_measure_stack_free(const char *stack);
+uintptr_t measure_stack_free_internal(const char *stack, size_t size);
 
 /**
  * @brief   Get the number of bytes used on the ISR stack
@@ -621,6 +626,24 @@ static inline const char *thread_get_name(const thread_t *thread)
 #endif
 }
 
+/**
+ * @brief       Measures the stack usage of a stack
+ *
+ * @pre         Only works if the thread was created with the flag
+ *              `THREAD_CREATE_STACKTEST`.
+ *
+ * @param[in] thread    The thread to measure the stack of
+ *
+ * @return              the amount of unused space of the thread's stack
+ */
+static inline uintptr_t thread_measure_stack_free(const thread_t *thread)
+{
+    /* explicitly casting void pointers is bad code style, but needed for C++
+     * compatibility */
+    return measure_stack_free_internal((const char *)thread_get_stackstart(thread),
+                                       thread_get_stacksize(thread));
+}
+
 #ifdef __cplusplus
 }
 #endif

@@ -171,15 +171,18 @@
     list->next = new_node;
 }
 
-uintptr_t thread_measure_stack_free(const char *stack)
+uintptr_t measure_stack_free_internal(const char *stack, size_t size)
 {
     /* Alignment of stack has been fixed (if needed) by thread_create(), so
      * we can silence -Wcast-align here */
     uintptr_t *stackp = (uintptr_t *)(uintptr_t)stack;
+    uintptr_t end = (uintptr_t)stack + size;
+
+    /* better be safe than sorry: align end of stack just in case */
+    end &= (sizeof(uintptr_t) - 1);
 
-    /* assume that the comparison fails before or after end of stack */
     /* assume that the stack grows "downwards" */
-    while (*stackp == (uintptr_t)stackp) {
+    while (((uintptr_t)stackp < end) && (*stackp == (uintptr_t)stackp)) {
         stackp++;
     }
 
@@ -240,7 +243,7 @@
    stacksize = (char *) thread->tls - stack;

    _init_tls(thread->tls);
 #endif

 #if defined(DEVELHELP) || defined(SCHED_TEST_STACK) \
    || defined(MODULE_TEST_UTILS_PRINT_STACK_USAGE)

@@ -197,7 +197,7 @@ UBaseType_t uxTaskGetStackHighWaterMark(TaskHandle_t xTask)
     thread_t *thread = thread_get((xTask == NULL) ? (uint32_t)thread_getpid()
                                                   : (uint32_t)xTask);
     assert(thread != NULL);
-    return thread_measure_stack_free(thread->stack_start);
+    return thread_measure_stack_free(thread);
 }
 
 void *pvTaskGetThreadLocalStoragePointer(TaskHandle_t xTaskToQuery,

@@ -92,10 +92,9 @@ void thread_isr_stack_init(void)
 
 int thread_isr_stack_usage(void)
 {
-    /* cppcheck-suppress comparePointers
-     * (reason: comes from ESP-SDK, so should be valid) */
-    return &port_IntStackTop - &port_IntStack -
-           thread_measure_stack_free((char*)&port_IntStack);
+    size_t stack_size = (uintptr_t)&port_IntStackTop - (uintptr_t)&port_IntStack;
+    return stack_size -
+           measure_stack_free_internal((char *)&port_IntStack, stack_size);
 }
 
 void *thread_isr_stack_pointer(void)

diff --git a/sys/ps/ps.c b/sys/ps/ps.c
@@ -98,7 +98,7 @@
 #ifdef DEVELHELP
             int stacksz = thread_get_stacksize(p);                          /* get stack size */
             overall_stacksz += stacksz;
-            int stack_free = thread_measure_stack_free(thread_get_stackstart(p));
+            int stack_free = thread_measure_stack_free(p);
             stacksz -= stack_free;
             overall_used += stacksz;
 #endif
@@ -128,11 +128,11 @@
 #endif
                   sname, queued, thread_get_priority(p)
 #ifdef DEVELHELP
                   , thread_get_stacksize(p), stacksz, stack_free,
                   thread_get_stackstart(p), thread_get_sp(p)
 #endif
 #ifdef MODULE_SCHEDSTATISTICS
                   , runtime_major, runtime_minor, switches, ztimer_us
 #endif
                  );
        }

diff --git a/sys/test_utils/print_stack_usage/print_stack_usage.c b/sys/test_utils/print_stack_usage/print_stack_usage.c
@@ -34,7 +34,7 @@
 
 void print_stack_usage_metric(const char *name, void *stack, unsigned max_size)
 {
-    unsigned free = thread_measure_stack_free(stack);
+    unsigned free = measure_stack_free_internal(stack, max_size);
 
     if ((LOG_LEVEL >= LOG_INFO) &&
         (thread_get_stacksize(thread_get_active()) >= MIN_SIZE)) {