Skip to content

Commit

Permalink
core/thread: fix thread_measure_stack_free()
Browse files Browse the repository at this point in the history
`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`.
  • Loading branch information
maribu committed May 31, 2024
1 parent 835eaee commit 3164824
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 15 deletions.
31 changes: 26 additions & 5 deletions core/include/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 internal_measure_stack_free(const char *stack, size_t size);

/**
* @brief Get the number of bytes used on the ISR stack
Expand Down Expand Up @@ -621,6 +626,22 @@ 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)
{
return internal_measure_stack_free(thread_get_stackstart(thread),
thread_get_stacksize(thread));
}

#ifdef __cplusplus
}
#endif
Expand Down
9 changes: 6 additions & 3 deletions core/thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 internal_measure_stack_free(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++;
}

Expand Down
2 changes: 1 addition & 1 deletion cpu/esp_common/freertos/task.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 3 additions & 4 deletions cpu/esp_common/thread_arch.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 -
internal_measure_stack_free((char *)&port_IntStack, stack_size);
}

void *thread_isr_stack_pointer(void)
Expand Down
2 changes: 1 addition & 1 deletion sys/ps/ps.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion sys/test_utils/print_stack_usage/print_stack_usage.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 = internal_measure_stack_free(stack, max_size);

if ((LOG_LEVEL >= LOG_INFO) &&
(thread_get_stacksize(thread_get_active()) >= MIN_SIZE)) {
Expand Down

0 comments on commit 3164824

Please sign in to comment.