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

Fix #2516, propagate stack pointer for child tasks #2517

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

jphickey
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Update CFE_ES_CreateChildTask to propagate the user-supplied stack pointer to the underlying OS_TaskCreate call. Also adds a functional test to check that the memory address of a local variable within a child task resides within the expected stack buffer.

NOTE: this requires an additional fix to POSIX OSAL to make it work on that platform.

Fixes #2516

Testing performed
Build and run all tests

Expected behavior changes
CFE Child tasks created will adhere to passed-in stack pointer.

System(s) tested on
Debian, RTEMS

Additional context
The newly added functional test will fail on POSIX due to a similar bug in the OS_TaskCreate() implementation. This didn't propagate the stack pointer, either.

The biggest risk with "fixing" this is that it might expose stack size problems that were otherwise hidden. Many apps use very small stack sizes for child tasks, and on POSIX (at least Linux/Glibc) it puts TLS storage in the stack buffer. This TLS takes about 10kB off the top of the stack. If the entire stack was only 4kB, this is now a problem. It was hidden because POSIX created a stack anyway, and when it created the stack it was at least 16kB. But by fixing that problem, the stack size really will be only 4kB, and thus not be big enough, and memory corruption will result.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

Related OSAL issue is fixed in nasa/osal#1450 (dependency)

CFE_ES_ExitChildTask();
}

void TestCreateChildWithStack(void)

Check notice

Code scanning / CodeQL

Long function without assertion Note test

All functions of more than 10 lines should have at least one assertion.
#define UT_LOCAL_STACK_SIZE 4096
static unsigned long UT_LOCAL_STACK[UT_LOCAL_STACK_SIZE];

void TestCheckStackPointer(void)

Check notice

Code scanning / CodeQL

Long function without assertion Note test

All functions of more than 10 lines should have at least one assertion.
@jphickey
Copy link
Contributor Author

Checked the failure reported in the workflows -- confirmed that it is the newly added test, and it should be fixed by the linked OSAL PR.

* NOTE: The custom stack does not work on RTEMS, test is disabled on that platform
* for the time being (custom stack may be deprecated in future CFE release).
*/
#ifndef _RTEMS_OS_

Check notice

Code scanning / CodeQL

Conditional compilation Note test

Use of conditional compilation must be kept to a minimum.
Update CFE_ES_CreateChildTask to propagate the user-supplied stack
pointer to the underlying OS_TaskCreate call.  Also adds a functional
test to check that the memory address of a local variable within a
child task resides within the expected stack buffer.

NOTE: this requires an additional fix to POSIX OSAL to make it work
on that platform.
@jphickey
Copy link
Contributor Author

jphickey commented Mar 6, 2024

Pushed the same content in new commit, in order to re-run workflows.

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 6, 2024
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Mar 7, 2024
dzbaker added a commit to nasa/cFS that referenced this pull request Mar 11, 2024
*Combines:*

cFE equuleus-rc1+dev104
to_lab equuleus-rc1+dev48
ci_lab equuleus-rc1+dev61
sample_app equuleus-rc1+dev40

**Includes:**

*cFE*
- nasa/cFE#2517

*to_lab*
- nasa/to_lab#194

*ci_lab*
- nasa/ci_lab#177

*sample_app*
- nasa/sample_app#229

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request Mar 11, 2024
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Mar 12, 2024
*Combines:*

cFE equuleus-rc1+dev107
to_lab equuleus-rc1+dev48
ci_lab equuleus-rc1+dev61
sample_app equuleus-rc1+dev40

**Includes:**

*cFE*
- nasa/cFE#2517
- nasa/cFE#2527

*to_lab*
- nasa/to_lab#194

*ci_lab*
- nasa/ci_lab#177

*sample_app*
- nasa/sample_app#229

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@dzbaker dzbaker merged commit 8c4cf9b into nasa:main Mar 12, 2024
22 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Mar 12, 2024
*Combines:*

cFE equuleus-rc1+dev107
to_lab equuleus-rc1+dev48
ci_lab equuleus-rc1+dev61
sample_app equuleus-rc1+dev40

**Includes:**

*cFE*
- nasa/cFE#2517
- nasa/cFE#2527

*to_lab*
- nasa/to_lab#194

*ci_lab*
- nasa/ci_lab#177

*sample_app*
- nasa/sample_app#229

Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@jphickey jphickey deleted the fix-2516-stackptr branch March 20, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CFE_ES_CreateChildTask ignores its StackPtr argument
2 participants