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

core/thread: error in thread.c corrected #21253

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DeJusten
Copy link

Contribution description

Errors in thread.c corrected:

  • in thread_wakeup() the status of the function to be woken up is incorrectly set to TASK_RUNNING. Instead the task should be set to STATUS_PENDING here, as only sched_run() is allowed to set a task to TASK_RUNNING.
  • in thread_create() it is checked whether the stack is large enough, but instead of aborting the function if the stack is too small and thus preventing a buffer overflow, only a debug output is made. Return statement added
  • in thread_state_to_string() it is not checked whether state is within the valid range, so that in the event of an error an invalid memory address is returned, which in turn leads to further invalid memory accesses when the string is output. thread_state_to_string() is used in particular by ps(). In the case of a corrupt thread context, which often occurred in my case due to the stack size being too small, thread_state_to_string() is called with an invalid status.

Other

  • thread_add_to_list(): Adding the task to the linked list is graphically commented (If you are interested, I also prepared this for core\lib\include\list.h and core\lib\include\clist.h)

Testing procedure

Issues/PRs references

- in thread_wakeup() the status of the function to be woken up is
  incorrectly set to TASK_RUNNING. Instead the task should be set
  to STATUS_PENDING here, as only sched_run() is allowed to set a
  task to TASK_RUNNING.
- in thread_create() it is checked whether the stack is large enough,
  but instead of aborting the function if the stack is too small and
  thus preventing a buffer overflow, only a debug output is made.
  Return statement added
- in thread_state_to_string() it is not checked whether state is within
  the valid range, so that in the event of an error an invalid
  memory address is returned, which in turn leads to further invalid
  memory accesses when the string is output. thread_state_to_string()
  is used in particular by ps(). In the case of a corrupt thread
  context, which often occurred in my case due to the stack size being
  too small, thread_state_to_string() is called with an invalid status.

Other
- thread_add_to_list(): Adding the task to the linked list is
  graphically commented (If you are interested, I also prepared this
  for core\lib\include\list.h and core\lib\include\clist.h)
@DeJusten DeJusten requested a review from kaspar030 as a code owner February 28, 2025 12:07
@github-actions github-actions bot added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Feb 28, 2025
@benpicco benpicco requested review from kfessel and maribu February 28, 2025 13:01
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 28, 2025
Remove Trailing Whitespace
@riot-ci
Copy link

riot-ci commented Feb 28, 2025

Murdock results

✔️ PASSED

fbf83a2 Update core/thread.c

Success Failures Total Runtime
10271 0 10271 09m:59s

Artifacts

Co-authored-by: Karl Fessel <[email protected]>
Comment on lines +399 to +400
assert(state < STATUS_NUMOF);
const char *name = state_names[state];
Copy link
Contributor

@kfessel kfessel Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is more reliable -- check for not exceeding NUMOF even if assertion are disabled

Suggested change
assert(state < STATUS_NUMOF);
const char *name = state_names[state];
const char *name = NULL;
if (state < STATUS_NUMOF){
name = state_names[state];
}
/* if compiling with assertions, this is an error that indicates that the table above is incomplete */
assert(name != NULL)
return (name != NULL) ? name : STATE_NAME_UNKNOWN;

Copy link
Contributor

@kfessel kfessel Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i could not add a suggestion to the unchanged code assert(name != NULL); -> i attached it to the first two lines

@mguetschow mguetschow added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Mar 4, 2025
@github-actions github-actions bot added the Process: missing approvals Integration Process: PR needs more ACKS (handled by action) label Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: missing approvals Integration Process: PR needs more ACKS (handled by action) Process: needs >1 ACK Integration Process: This PR requires more than one ACK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants