Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a stack pointer bounds check when configCHECK_FOR_STACK_OVERFLOW is set to 2. #1216

Merged
merged 4 commits into from
Dec 30, 2024

Conversation

jiladahe1997
Copy link
Contributor

Add a stack pointer bounds check when configCHECK_FOR_STACK_OVERFLOW is set to 2.

Description

I'm working on a small STM32F4 project using FreeRTOS. I've encountered a problem with stack overflow checks:

I am unsure how many bytes I need for my shell thread, so I set the stack size to 1024 bytes.

As documented in FreeRTOS-Kernel/include/stack_macros.h:

Setting configCHECK_FOR_STACK_OVERFLOW to 1 will cause the macro to check
the current stack state only - comparing the current top of stack value to
the stack limit. Setting configCHECK_FOR_STACK_OVERFLOW to greater than 1
will also cause the last few stack bytes to be checked to ensure the value
to which the bytes were set when the task was created have not been
overwritten. Note this second test does not guarantee that an overflowed
stack will always be recognised.

And as it says, setting configCHECK_FOR_STACK_OVERFLOW = 2 "will also" check the last few stack bytes. This suggests it should check both the stack bounds and the last few bytes.

However, here’s the problem I encountered:

int do_something(){
     uint8_t big_array[1024];
     ......
}
void shell_thread(void *parameters)
{
   ......
   ret = do_something();
   ......
}
#define SHELL_TASK_STACK_SIZE 256
StackType_t xShellStack[SHELL_TASK_STACK_SIZE];
xHandle = xTaskCreateStatic(shellTask,
                            "shell", 
                            SHELL_TASK_STACK_SIZE,
                            &shell, 
                            tskIDLE_PRIORITY, 
                            xShellStack,
                            &xTaskBuffer);

In this scenario, the FreeRTOS kernel crashes, and the MCU enters a hard fault. The stack overflow check fails to detect the issue because configCHECK_FOR_STACK_OVERFLOW = 2 only verifies whether the last few stack bytes have been overwritten. However, in the above scenario, the stack has overflowed without overwriting the monitored bytes.

This patch fixes the issue by adding a stack pointer bounds check when configCHECK_FOR_STACK_OVERFLOW is set to 2. This improves the reliability of stack overflow detection and also aligns with the behavior described in stack_macros.h.



*Side Effects
Adding the stack pointer bounds check introduces extra overhead during the stack overflow detection process, which may slightly reduce performance.

Test Steps

  1. Set configCHECK_FOR_STACK_OVERFLOW = 2.
  2. Create a task with a stack and allocate a large array on the stack without initializing it.
  3. Verify that the program correctly detects the overflow and invokes vApplicationStackOverflowHook.

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jiladahe1997 jiladahe1997 requested a review from a team as a code owner December 26, 2024 05:56
@jasonpcarroll
Copy link
Member

Hi @jiladahe1997,

Thank you for bringing this to our attention and for the PR. Code looks fine. You may have to fix up the formatting and unit tests. I will check back here in a bit.

Best,

Jason Carroll

@jiladahe1997 jiladahe1997 force-pushed the dev_stack_pointer_check branch from 044d5b2 to abef464 Compare December 28, 2024 00:30
@jiladahe1997
Copy link
Contributor Author

Hi @jiladahe1997,

Thank you for bringing this to our attention and for the PR. Code looks fine. You may have to fix up the formatting and unit tests. I will check back here in a bit.

Best,

Jason Carroll

Hi @jasonpcarroll,

Thanks for your reply. I noticed a format-check error in the GitHub CI, but it seems there were no unit test errors. I have fixed the format issue. Please review it again.

@jasonpcarroll
Copy link
Member

jasonpcarroll commented Dec 30, 2024

I will re-run to confirm formatting and approve and then will get someone else to take a look. Thanks!

@jiladahe1997
Copy link
Contributor Author

I will re-run to confirm formatting and approve and then will get someobe else to take a look. Thanks!

Appreciate the follow-up! Let me know if there’s anything else to address. Looking forward to the other reviewer’s feedback.

jasonpcarroll
jasonpcarroll previously approved these changes Dec 30, 2024
Signed-off-by: Gaurav Aggarwal <[email protected]>
aggarg
aggarg previously approved these changes Dec 30, 2024
Signed-off-by: Gaurav Aggarwal <[email protected]>
@aggarg aggarg merged commit e55bde2 into FreeRTOS:main Dec 30, 2024
17 checks passed
@aggarg
Copy link
Member

aggarg commented Dec 30, 2024

@jiladahe1997 Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants