Skip to content

Conversation

@pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Jun 12, 2020

The version as shipped in Newlib itself is coded a bit sloppily for an
embedded environment. We thus want to override it (and make it weak, to
allow user apps to override it in turn, if needed). The desired
properties of the implementation are:

  1. It should call _write() (Newlib implementation calls write()).
  2. It should be minimal (Newlib implementation allocates message
    on the stack, i.e. misses "static const").

Signed-off-by: Paul Sokolovsky [email protected]

@pfalcon pfalcon added area: C Library C Standard Library area: Minimal libc Minimal C Standard Library labels Jun 12, 2020
@pfalcon pfalcon requested a review from galak June 12, 2020 08:29
@pfalcon pfalcon requested review from cfriedt and stephanosio June 12, 2020 08:29
@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 12, 2020

This is related to #25479. More discussion is available in zephyrproject-rtos/sdk-ng#221

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

Need to add some kind of infinite loop at the end to make sure that this function does not return as per FUNC_NORETURN.

error: 'noreturn' function does return [-Werror]

EDIT: CODE_UNREACHABLE should do.

@cfriedt
Copy link
Member

cfriedt commented Jun 16, 2020

I wonder if there is a per-thread exit that a thread is allowed to call on itself with Zephyr. That would satisfy the noreturn requirement and might also allow for memory to be reused.

Ahh... too bad:

A thread may asynchronously end its execution by aborting. The kernel automatically aborts a thread if the thread triggers a fatal error condition, such as dereferencing a null pointer.

A thread can also be aborted by another thread (or by itself) by calling k_thread_abort(). However, it is typically preferable to signal a thread to terminate itself gracefully, rather than aborting it.

As with thread termination, the kernel does not reclaim shared resources owned by an aborted thread.

https://docs.zephyrproject.org/latest/reference/kernel/threads/index.html#thread-aborting

@stephanosio
Copy link
Member

stephanosio commented Jun 16, 2020

I wonder if there is a per-thread exit that a thread is allowed to call on itself with Zephyr. That would satisfy the noreturn requirement and might also allow for memory to be reused.

This warning/error here is just a compiler construct, because k_oops is not marked noreturn.

p.s. If CONFIG_USERSPACE is enabled, aborting a thread will additionally mark the thread and stack objects as uninitialized so that they may be re-used. If the faulting thread is not a user thread, it makes no sense to continue executing as the system is already compromised and that warrants a complete reboot.

@cfriedt
Copy link
Member

cfriedt commented Jun 16, 2020

@stephanosio - agreed.
So

#ifndef CONFIG_USERSPACE
k_thread_abort(k_current_get());
#endif

?

@stephanosio
Copy link
Member

@cfriedt While you can do that, that should not be necessary:

FUNC_NORETURN void z_thread_entry(k_thread_entry_t entry,
void *p1, void *p2, void *p3)
{
entry(p1, p2, p3);
k_thread_abort(k_current_get());
/*
* Compiler can't tell that k_thread_abort() won't return and issues a
* warning unless we tell it that control never gets this far.
*/
CODE_UNREACHABLE; /* LCOV_EXCL_LINE */
}

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 16, 2020

This warning/error here is just a compiler construct, because k_oops is not marked noreturn.

So, that's where the problem lies, and what needs to be fixed.

@stephanosio
Copy link
Member

So, that's where the problem lies, and what needs to be fixed.

Unfortunately, that would be easier said than done due to how it is implemented.

@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 16, 2020

So, that's where the problem lies, and what needs to be fixed.

Ah, ok, it's proverbial Zephyr with its demigod designers (71ce8ce):

 * On architectures where k_thread_abort() never returns, this function
 * never returns either.
 *
 * @param reason The reason for the fatal error
 * @param esf Exception context, with details and partial or full register
 *            state when the error occurred. May in some cases be NULL.
 */
void z_fatal_error(unsigned int reason, const z_arch_esf_t *esf);

Because you see, on some architectures, after a thread is aborted, it makes sense to continue executing its code after the abort. Makes it all more secure. And probably a lot of TOC, TOU, SGX, CBA, FGH, XYZ, and other TLAs involved.

The version as shipped in Newlib itself is coded a bit sloppily for an
embedded environment. We thus want to override it (and make it weak, to
allow user apps to override it in turn, if needed). The desired
properties of the implementation are:

1. It should call _write() (Newlib implementation calls write()).
2. It should be minimal (Newlib implementation allocates message
on the stack, i.e. misses "static const").

Signed-off-by: Paul Sokolovsky <[email protected]>
@pfalcon
Copy link
Contributor Author

pfalcon commented Jun 16, 2020

EDIT: CODE_UNREACHABLE should do.

Added.

@pfalcon pfalcon closed this Jun 16, 2020
@pfalcon pfalcon reopened this Jun 16, 2020
@carlescufi carlescufi merged commit 5f05d65 into zephyrproject-rtos:master Jun 17, 2020
@stephanosio stephanosio added area: newlib Newlib C Standard Library and removed area: Minimal libc Minimal C Standard Library labels Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: C Library C Standard Library area: newlib Newlib C Standard Library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants