core/thread: fix thread_measure_stack_free()

maribu · maribu · commit e93b5e4b98a8 · 2024-05-31T19:54:10.000+02:00
`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`.
diff --git a/core/include/thread.h b/core/include/thread.h
@@ -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
diff --git a/core/thread.c b/core/thread.c
@@ -171,15 +171,18 @@ void thread_add_to_list(list_node_t *list, thread_t *thread)
     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++;
     }
 
diff --git a/cpu/esp_common/freertos/task.c b/cpu/esp_common/freertos/task.c
@@ -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,
diff --git a/cpu/esp_common/thread_arch.c b/cpu/esp_common/thread_arch.c
@@ -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 @@ void ps(void)
 #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
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)) {

Original file line number	Diff line number	Diff line change
`@@ -171,15 +171,18 @@ void thread_add_to_list(list_node_t list, thread_t thread)`
`171`	`171`	`list->next = new_node;`
`172`	`172`	`}`
`173`	`173`
`174`		`-uintptr_t thread_measure_stack_free(const char *stack)`
	`174`	`+uintptr_t measure_stack_free_internal(const char *stack, size_t size)`
`175`	`175`	`{`
`176`	`176`	`/* Alignment of stack has been fixed (if needed) by thread_create(), so`
`177`	`177`	`* we can silence -Wcast-align here */`
`178`	`178`	`uintptr_t stackp = (uintptr_t )(uintptr_t)stack;`
	`179`	`+ uintptr_t end = (uintptr_t)stack + size;`
	`180`	`+`
	`181`	`+ /* better be safe than sorry: align end of stack just in case */`
	`182`	`+ end &= (sizeof(uintptr_t) - 1);`
`179`	`183`
`180`		`- /* assume that the comparison fails before or after end of stack */`
`181`	`184`	`/* assume that the stack grows "downwards" */`
`182`		`- while (*stackp == (uintptr_t)stackp) {`
	`185`	`+ while (((uintptr_t)stackp < end) && (*stackp == (uintptr_t)stackp)) {`
`183`	`186`	`stackp++;`
`184`	`187`	`}`
`185`	`188`
Original file line number	Diff line number	Diff line change
`@@ -197,7 +197,7 @@ UBaseType_t uxTaskGetStackHighWaterMark(TaskHandle_t xTask)`
`197`	`197`	`thread_t *thread = thread_get((xTask == NULL) ? (uint32_t)thread_getpid()`
`198`	`198`	`: (uint32_t)xTask);`
`199`	`199`	`assert(thread != NULL);`
`200`		`- return thread_measure_stack_free(thread->stack_start);`
	`200`	`+ return thread_measure_stack_free(thread);`
`201`	`201`	`}`
`202`	`202`
`203`	`203`	`void *pvTaskGetThreadLocalStoragePointer(TaskHandle_t xTaskToQuery,`
Original file line number	Diff line number	Diff line change
`@@ -92,10 +92,9 @@ void thread_isr_stack_init(void)`
`92`	`92`
`93`	`93`	`int thread_isr_stack_usage(void)`
`94`	`94`	`{`
`95`		`- /* cppcheck-suppress comparePointers`
`96`		`- * (reason: comes from ESP-SDK, so should be valid) */`
`97`		`- return &port_IntStackTop - &port_IntStack -`
`98`		`- thread_measure_stack_free((char*)&port_IntStack);`
	`95`	`+ size_t stack_size = (uintptr_t)&port_IntStackTop - (uintptr_t)&port_IntStack;`
	`96`	`+ return stack_size -`
	`97`	`+ measure_stack_free_internal((char *)&port_IntStack, stack_size);`
`99`	`98`	`}`
`100`	`99`
`101`	`100`	`void *thread_isr_stack_pointer(void)`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@`
`34`	`34`
`35`	`35`	`void print_stack_usage_metric(const char name, void stack, unsigned max_size)`
`36`	`36`	`{`
`37`		`- unsigned free = thread_measure_stack_free(stack);`
	`37`	`+ unsigned free = measure_stack_free_internal(stack, max_size);`
`38`	`38`
`39`	`39`	`if ((LOG_LEVEL >= LOG_INFO) &&`
`40`	`40`	`(thread_get_stacksize(thread_get_active()) >= MIN_SIZE)) {`