libc: newlib: assert: Override handler for C std assert

[hubertmis]

Libraries using standard C assert from <assert.h> did not give useful
information on assertion fault, as default newlib implementation tries
to print assert info to stderr. It causes another assertion in Zephyr.

With this patch, the default handler is overridden to use native Zephyr
assert macros and give useful info when assertion fails.

[zephyrbot]

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[stephanosio]

Libraries using standard C assert from <assert.h> did not give useful
information on assertion fault, as default newlib implementation tries
to print assert info to stderr. It causes another assertion in Zephyr.

I am not seeing that with the Zephyr SDK.

Sample assert output with the default newlib implementation:

assertion "false" failed: file "../src/main.c", line 16, function: main

What toolchain and board are you using?

[hubertmis]

I'm using GCC in Ubuntu 18.04, compiling projects for nRF52840-DK.
I don't have any examples to share right now. I can paste them next week.

Perhaps something is missing in my project that makes _vfprintf_r of similar function cause Zephyr assertion fault. With the patch I proposed, libc assertion faults are handled correctly in Zephyr even if _vfprintf_r does not work as expected.

[stephanosio]

I'm using GCC

I suppose you are referring to the GCC provided as part of the GNU ARM Embedded toolchain?

Perhaps something is missing in my project that makes _vfprintf_r of similar function cause Zephyr assertion fault. With the patch I proposed, libc assertion faults are handled correctly in Zephyr even if _vfprintf_r does not work as expected.

the _vfprintf_r will eventually call _write provided by libc-hooks.c and the default implementation does not care which fd is specified. Did you override the default _write implementation by any chance?

[stephanosio]

I'm using GCC in Ubuntu 18.04

If you are using the Ubuntu gcc-arm-none-eabi package, I am not sure that was ever intended to work. The only "officially supported" ARM toolchains are the one from Zephyr SDK and the GNU ARM Embedded from ARM.

cc @tejlmand

[hubertmis]

I suppose you are referring to the GCC provided as part of the GNU ARM Embedded toolchain?

Yes, that's correct assumption. I've installed it from https://developer.arm.com/

Did you override the default _write implementation by any chance?

Not intentionally. It is possible that one of the dependencies of my project did it.

I thought that missing assert info is a common problem for newlib users in Zephyr. Apparently, it's not. I'll spend some more time next week to verify what causes problem in my project and provide more info then.

[hubertmis]

@stephanosio Or is it possible that calling _write from IRQ context causes Zephyr assertion fault? Perhaps it uses some kernel functions that are not allowed in the IRQ context. I think I've seen the problem when assertion fault was hit in IRQs.

[stephanosio]

@stephanosio Or is it possible that calling _write from IRQ context causes Zephyr assertion fault? Perhaps it uses some kernel functions that are not allowed in the IRQ context. I think I've seen the problem when assertion fault was hit in IRQs.

That sounds probable, especially if the UART type you are using is either interrupt-driven or async.

[hubertmis]

What would be the correct way to address this problem?
It looks that this patch solves the issue, as __ASSERT macros can be called from IRQs. Alternatively, _write implementation could be changed to make sure it is OK to call it from IRQs. Are there any other options?

[pfalcon]

Or is it possible that calling _write from IRQ context causes Zephyr assertion fault?

@hubertmis, You definitely should not expect POSIX, or POSIX-like functions to work from IRQ context. POSIX (and ANSI C) simply don't define something like "IRQ context", and thus doesn't specify which C/POSIX functions work from it.

If you work with (Zephyr) IRQ contexts, you should use Zephyr functions suitable for that. One obvious candidate is printk(). (But mind that its behavior may depend on Zephyr config options, see e.g. #10715 (that particular PR was closed, but I'm not sure if that or similar change may still get thru)).

[hubertmis]

@pfalcon , Thanks for clarification. In such circumstances it seems to me that the patch that I proposed in this PR is a valid solution to get correct assert info from both thread and IRQ contexts.

[pfalcon]

@hubertmis: Thanks for looking into this issue. It so happens that recently we indeed find that we need to override some Newlib aspects, to make them more suitable for Zephyr applications, e.g. #26135 .

This change is in the same vein, so I imagine it might be needed. But I share concern of @stephanosio that we should be sure that in this particular case this change is needed, and would be an improvement. I'd suggest that such a change should include a sample, samples/libc/assert/, which would demonstrate an assert() call, so reviewers can compare its behavior before and after.

Second concern is the actual implementation. This patch predicates it on CONFIG_ASSERT, but what's the default setting for that option? (Please check.)

When I write my ANSI C application, I want it behave in predictable ways, based on de-jure or de-facto standard. I don't want it do depend (disclaimer: whenever possible) on the obscure config options of the underlying vendor RTOS. What I expect of assert() is: a) it's enabled by default; b) unless I built my project with -DNDEBUG. The implementation you provide here doesn't agree with those expectations.

Also added @cfriedt to reviewers, who seem to have even stronger opinion of how C stdlib should work in Zephyr.

[pfalcon]

@pfalcon , Thanks for clarification. In such circumstances it seems to me that the patch that I proposed in this PR is a valid solution to get correct assert info from both thread and IRQ contexts.

@hubertmis, Thanks for the response, and I'm not sure about the "IRQ contexts" part. In my view, assert() and "IRQ contexts" are orthogonal notions, not mixing together. In other words, you should not expect that if you call assert() in an IRQ context, you'll get a particular behavior (effectively, it's undefined behavior). Of course, we'd like to make it work in a reasonable way in this case - whenever possible, and not at the expense of the normal usage of assert() (and normal usage includes being enabled by default, outputting to stderr, which may be redirected (we don't readily support that (redirection) in Zephyr as of now, but likely will, as we grow more POSIX functionality)).

[hubertmis]

I don't think I understand the full picture here. I've seen in documentation following statement:

The __ASSERT() macro can be used inside kernel and application code to perform optional runtime checks

I understand that libraries and applications, libraries, or kernel modules in Zephyr are expected to use __ASSERT() macro. However, some OS-agnostic libraries use assert() from stdlib instead. My intention was to make such OS-agnostic libraries follow Zephyr expectations when running in Zephyr, to help with debugging assertion faults in IRQs.

Have I misunderstood the purpose of __ASSERT() and assert() macros?

[hubertmis]

@pfalcon,

What I expect of assert() is: a) it's enabled by default; b) unless I built my project with -DNDEBUG. The implementation you provide here doesn't agree with those expectations.

The implementation I provided binds assert() with __ASSERT() if __ASSERT() is available. If __ASSERT() is not available, default weak implementation is still present and enabled by default.

[hubertmis]

More info from my investigation. When I run a modified sample to cause assertion fault in ISR I get following message on UART console:

uart:~$ ASSERTION FAIL [((arch_is_in_isr() == 0) || ((timeout).ticks == (((k_timeout_t) {})).ticks))] @ zephyr/kernel/sem.c:141

E: ***** HARD FAULT *****
E:   Fault escalation (see below)
E: r0/a1:  0x00000004  r1/a2:  0x0000008d  r2/a3:  0x00000001
E: r3/a4:  0x000069a9 r12/ip:  0xaaaaaaaa r14/lr:  0x00015b7b
E:  xpsr:  0x61000011
E: Faulting instruction address (r15/pc): 0x0002eeda
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
E: Fault during interrupt handling

E: Current thread: 0x20008b5c (unknown)
E: Resetting system

with following backtrace:

#2  0x000139ae in z_fatal_error (reason=reason@entry=0, esf=esf@entry=0x2000f1e8 <z_interrupt_stacks+1608>) at zephyr/kernel/fatal.c:123
#3  0x000076aa in z_arm_fatal_error (reason=reason@entry=0, esf=esf@entry=0x2000f1e8 <z_interrupt_stacks+1608>) at zephyr/arch/arm/core/aarch32/fatal.c:47
#4  0x00007ae0 in z_arm_fault (msp=<optimized out>, psp=<optimized out>, exc_return=<optimized out>) at zephyr/arch/arm/core/aarch32/cortex_m/fault.c:969
#5  0x00007794 in z_arm_usage_fault () at zephyr/arch/arm/core/aarch32/cortex_m/fault_s.S:85
#6  <signal handler called>
#7  assert_post_action (file=0x4 <z_finalize_fd+4> "Iw", file@entry=0x47a33 "zephyr/kernel/sem.c", line=line@entry=141) at zephyr/lib/os/assert.c:46
#8  0x00015b7a in z_impl_k_sem_take (sem=sem@entry=0x200007b0 <heap_sem>, timeout=...) at zephyr/kernel/sem.c:140
#9  0x0002e8ee in k_sem_take (timeout=..., sem=sem@entry=0x200007b0 <heap_sem>) at zephyr/include/generated/syscalls/kernel.h:756
#10 sys_sem_take (sem=sem@entry=0x200007b0 <heap_sem>, timeout=...) at zephyr/lib/os/sem.c:127
#11 0x00007f7e in _sbrk (count=0) at zephyr/lib/libc/newlib/libc-hooks.c:252
#12 0x000183f8 in _sbrk_r ()
#13 0x00017f22 in _malloc_r ()
#14 0x000334ce in __sfmoreglue ()
#15 0x00018846 in __sfp ()
#16 0x000187d2 in __sinit ()
#17 0x00017fb4 in _vfprintf_r ()
#18 0x00017e12 in fprintf ()
#19 0x00017da8 in __assert_func ()

When I apply patch from this PR I get following (expected) output:

uart:~$ ASSERTION FAIL [...] @ zephyr/lib/libc/newlib/assert.c:18
        Assertion fail: (1 == 0) at modules/hal/nordic/drivers/nrf_radio_802154/src/nrf_802154_trx.c:2541
E: ***** HARD FAULT *****
E:   Fault escalation (see below)
E: r0/a1:  0x00000004  r1/a2:  0x00000015  r2/a3:  0x00000001
E: r3/a4:  0x00046124 r12/ip:  0xaaaaaaaa r14/lr:  0x00011401
E:  xpsr:  0x61000011
E: Faulting instruction address (r15/pc): 0x0002e6da
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 0
E: Fault during interrupt handling

E: Current thread: 0x20008b5c (unknown)
E: Resetting system

When I force my program to run assertion fault from a thread I get following (expected) output without the patch:

assertion "0 == 1" failed: file "modules/hal/nordic/drivers/nrf_radio_802154/src/nrf_802154.c", line 458, function: nrf_802154_receive

And similar one with the patch applied:

ASSERTION FAIL [...] @ zephyr/lib/libc/newlib/assert.c:18
        Assertion fail: (0 == 1) at modules/hal/nordic/drivers/nrf_radio_802154/src/nrf_802154.c:458
E: r0/a1:  0x00000004  r1/a2:  0x00000015  r2/a3:  0x00000001
E: r3/a4:  0x000459ad r12/ip:  0x00000030 r14/lr:  0x0000f5e5
E:  xpsr:  0x61000000
E: Faulting instruction address (r15/pc): 0x0002e712
E: >>> ZEPHYR FATAL ERROR 4: Kernel panic on CPU 0
E: Current thread: 0x20008bd0 (unknown)
E: Resetting system

[stephanosio]

@hubertmis According to the backtrace you provided, the fault is from the malloc call inside fprintf, meaning it has nothing to do with the UART driver (or assert function for that matter):

#11 0x00007f7e in _sbrk (count=0) at zephyr/lib/libc/newlib/libc-hooks.c:252
#12 0x000183f8 in _sbrk_r ()
#13 0x00017f22 in _malloc_r ()

What happens is that fprintf calls malloc, and the currently allocated heap size happens to be smaller than the required size, so it calls _sbrk to extend the heap (i.e. this issue should be intermittent).

_sbrk calls sys_sem_take, which calls k_sem_take when userspace is not enabled, and k_sem_take cannot be called inside an ISR:

zephyr/kernel/sem.c

Lines 140 to 141 in e39bf29

	__ASSERT(((arch_is_in_isr() == false) ||
	K_TIMEOUT_EQ(timeout, K_NO_WAIT)), "");

This effectively makes fprintf unsafe to call from an ISR.

[stephanosio]

One solution to this problem is to replace the following critical section implementation with a spinlock:

zephyr/lib/libc/newlib/libc-hooks.c

Lines 252 to 267 in 45979da

	sys_sem_take(&heap_sem, K_FOREVER);
	#if CONFIG_NEWLIB_LIBC_ALIGNED_HEAP_SIZE
	ptr = heap_base + heap_sz;
	#else
	ptr = ((char *)HEAP_BASE) + heap_sz;
	#endif
	if ((heap_sz + count) < MAX_HEAP_SIZE) {
	heap_sz += count;
	ret = ptr;
	} else {
	ret = (void *)-1;
	}
	sys_sem_give(&heap_sem);

The impact of using spinlock here should be minimal since it only consists of tiny arithmetic operations.

[andrewboie]

One solution to this problem is to replace the following critical section implementation with a spinlock:

I don't think the C library code should be using kernel primitives like spinlocks at all. If you put spinlocks here it will break any user mode code.

This effectively makes fprintf unsafe to call from an ISR.

Let's step back for a second. I don't think this code should be safe from an ISR. Why would we need this to work from an ISR? ISR is a very special execution environment, only a limited subset of functions should be available, with lots of things not possible...to include any code which is making any potentially blocking calls to get more heap memory!

k_mem_pool() and k_heap() exist for managing heaps from kernel code. If ISR code is somehow (either directly or transitively) invoking C library malloc() that's simply wrong.

[stephanosio]

I don't think the C library code should be using kernel primitives like spinlocks at all. If you put spinlocks here it will break any user mode code.

@andrewboie Sure, for user-mode implementation, futex should be used.

Let's step back for a second. I don't think this code should be safe from an ISR.

The simplest and possibly the most sane solution is to declare assert must not be used inside an ISR and __ASSERT should be used instead. Let's see if others agree.

[andrewboie]

The simplest and possibly the most sane solution is to declare assert must not be used inside an ISR and __ASSERT should be used instead. Let's see if others agree.

Yes that's the only way to go. Under no circumstances should we expect the entire set of libc functions to work in interrupt context. You cannot do any synchronization without using spinlocks.

[andrewboie]

Also, in addition to synchronization problems there are also security concerns. The kernel and drivers should never ever use the libc malloc heap for ANY reason. The heap could be deliberately corrupted by user mode to get the kernel to reveal sensitive information, and user threads would have access to all such allocations. We would at minimum need a separate brk() arena for the kernel itself, and we would still have synchronization problems even if that were the case.

[stephanosio]

The kernel and drivers should never ever use the libc malloc heap for ANY reason.

@andrewboie IIUC, @hubertmis is having this problem in his (possibly common; as in, can be called from both thread and ISR contexts) application code that is called by one or more callback functions that happen to be called from an ISR.

In that sense, we should probably look into reworking how callbacks function in Zephyr (e.g. instead of directly calling the callback function inside an ISR, we should be reserving a worker thread and dispatching callbacks to the worker thread's work queue).

We can even argue the current callback scheme in itself is a security risk.

[andrewboie]

The kernel and drivers should never ever use the libc malloc heap for ANY reason.

@andrewboie IIUC, @hubertmis is having this problem in his (possibly common; as in, can be called from both thread and ISR contexts) application code that is called by one or more callback functions that happen to be called from an ISR.

In that sense, we should probably look into reworking how callbacks function in Zephyr (e.g. instead of directly calling the callback function inside an ISR, we should be reserving a worker thread and dispatching callbacks to the worker thread's work queue).

We can even argue the current callback scheme in itself is a security risk.

Agreed, there is much work to do in this area. With a proper kernel/application split, installing callbacks becomes deeply problematic. Some thoughts about this here: #25950

I don't have a good sense of a solution yet, but I'd like it to not have any footprint implications for code that isn't using any memory protection, basically the tiniest of micro-controllers.

[andrewboie]

One possible idea to consider: if (and only if) synchronization is only done with spinlocks, and we don't use memory protection, then a larger subset of libc functionality might be workable in ISRs (as long as it doesn't sleep).

It may be the case that we might macro some current usages of sys_sem (or similar usermode friendly locking primitives) in libc code with spinlocks if CONFIG_USERSPACE=n. That would make life easier for people who don't care about user mode considerations. Would need to be done very carefully.

IIRC, a sys_sem and a k_sem are the same thing if CONFIG_USERSPACE=n. So for this particular issue we would need to use a spinlock instead. I am unsure how far down this rabbit hole we want to go, @andyross should be part of the conversation.

The problem with spinlocks, of course, is that they impact system latency so we prefer other kinds of locking if feasible.

[hubertmis]

@stephanosio,

application code that is called by one or more callback functions that happen to be called from an ISR

That's not exactly true in my application. It looks that calls from the driver are moved from ISR to a thread:

zephyr/drivers/ieee802154/ieee802154_nrf5.c

Line 624 in 3f8bdf1

k_sem_give(&nrf5_data.tx_wait);

I think the problem here is that the driver itself uses assert()s from stdlib instead of Zephyr __ASSERTS(), like here:
https://github.com/NordicSemiconductor/nRF-IEEE-802.15.4-radio-driver/blob/61bc74d7fb420779e58795581c0b53a0531c89c1/src/nrf_802154_core.c#L1836

We cannot rewrite the driver to use Zephyr macros, as this driver is OS independent.

Would it be possible to declare assert() as a function from stdlib that can be called in ISRs and provide valid implementation of __assert_func - to make sure that calling it in ISRs is safe? Are there any issues with such solution?

[hubertmis]

I think that the example I was using for my test might be heapless. Even if we made it possible to use malloc in ISRs (what I consider a bad idea), it would return failure and assert() message would be still unknown.

If I understand what you suggested, there are only two ways to solve:

1. Prevent using assert() in drivers and force worker threads for calls from drivers to applications.
2. Make assert() implementation safe to call in ISRs.

I think that option 2 is easier to achieve.

[stephanosio]

We cannot rewrite the driver to use Zephyr macros, as this driver is OS independent.

well, this turned out to be a hairier situation than I thought.

Would it be possible to declare assert() as a function from stdlib that can be called in ISRs and provide valid implementation of __assert_func - to make sure that calling it in ISRs is safe? Are there any issues with such solution?

This looks to be the most sane solution indeed. After all, we allow C library functions like memcpy and memcmp in the ISRs -- no reason assert can't be one of them, as long as it does not internally call functions like fprintf and malloc.

[stephanosio]

Instead of having this function in a separate file, given that it is unlikely we will ever add more assert-related stuff here, can we move it into libc-hooks.c as with the rest of the newlib hook functions?

[pfalcon]

@hubertmis

The implementation I provided binds assert() with __ASSERT() if __ASSERT() is available. If __ASSERT() is not available, default weak implementation is still present and enabled by default.

That only makes the situation worse IMHO. Not only assert() doesn't behave as expected re: outputting to stderr, etc., how it behaves now depends on (generally unrelated) config option, i.e. if a user flips that option, application behavior noticeably changes (for no apparent to a user reasons).

[pfalcon]

I think the problem here is that the driver itself uses assert()s from stdlib instead of Zephyr __ASSERTS(), like here:
https://github.com/NordicSemiconductor/nRF-IEEE-802.15.4-radio-driver/blob/61bc74d7fb420779e58795581c0b53a0531c89c1/src/nrf_802154_core.c#L1836

@hubertmis, so you pinpointed the problem. Instead of assert(), which is again, an ANSI C function for application environments (not drivers, not OS kernel, not etc), there should be something like NORDIC_NRF_ASSERT(), and then then somewhere in headers:

#ifndef NORDIC_NRF_ASSERT
#define NORDIC_NRF_ASSERT(...) <suitable default>
#endif

I.e., allow users of this driver to override definition of NORDIC_NRF_ASSERT(). In case of Zephyr, you would
define it to __ASSERT().

I hope the situation is clear now, and I would be ready to give -1 to this patch.

[pfalcon]

Based on the discussion, the proposed patch is intended to work around an issue in a Nordic nRF BSP (or standalone driver). It's not a good approach to work around problems with out-of-tree drivers in the OS code in general, and this particular patch would affect other (legitimate) users of the ANSI C assert() function.

Given that @hubertmis is from Nordic, and Nordic as among Zephyr founding members, I'm sure a better solution can be found (one suggestion is in a comment above).

Otherwise, following @stephanosio, I tested how assert() works currently, and it works as expected for both ZEPHYR_TOOLCHAIN_VARIANT=zephyr and ZEPHYR_TOOLCHAIN_VARIANT=gnuarmemb.

So, let me give -1 on this, and call to look for a different solution.

Note that if circumstances are such that a proper solution will require some time, and an interim workaround is required (which is known to happen), I'd suggest:

1. To make a separate, and clearly named config option for the workaround, e.g. CONFIG_WORKAROUND_NORDIC_BSP_ASSERT, to make clear the purpose and scope of this option.
2. To keep it in a separate file, to easier (without extra git noise) remove once it's no longer needed.

Thanks.

[hubertmis]

@pfalcon, I understand your concern. We can update the 802.15.4 driver within reasonable timeframe and because of that I think no workaround is needed.

The issue will be still visible if any other driver uses C std assert() or call callback to application directly. But as it is not bothering me at the moment, I'm closing this PR.

We'll prepare an updated 802.15.4 that does not call assert() and open a new PR with driver update.

[pfalcon]

@hubertmis

We can update the 802.15.4 driver within reasonable timeframe and because of that I think no workaround is needed.

Thanks!

The issue will be still visible if any other driver uses C std assert() or call callback to application directly. But as it is not bothering me at the moment, I'm closing this PR.

Well, then they will hopefully find this ticket by search, or will be referred to it by other developers. And we can see how common the problem is and whether it really makes sense to provide some workarounds on Zephyr side.