_REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

_REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

Hallo all:

I have used Newlib 3.1.0 (and older versions) for quite some time. I am trying to upgrade to the latest release/snapshot 3.3.0, but I hit a compilation error in release builds (with NDEBUG defined):

newlib-3.3.0/newlib/libc/stdlib/rand.c:78: undefined reference to `__assert_func'

I tracked it down to this definition:

/* Specify how to handle reent_check malloc failures. */
#ifdef _REENT_CHECK_VERIFY
#include <assert.h>
#define __reent_assert(x) ((x) ? (void)0 : __assert_func(__FILE__, __LINE__, (char *)0, "REENT malloc succeeded"))
#else
#define __reent_assert(x) ((void)0)
#endif

This is unfortunate. First of all, I wonder what happens if malloc fails and there is no assert. Will there be a crash?

Then, I would like to assert() in debug builds, and not in release builds. My code does not define __assert_func in release builds, because assertions are only supposed to work if NDEBUG is not defined. That has been working fine for years, until this Newlib version.

I am configuring Newlib with --disable-newlib-multithread , because my embedded firmware has no threads. But I guess I still have to deal with "struct _reent", don't I? I would have hoped that, in this single-thread situation, any reentrancy structure could be allocated statically. Or is there any way to avoid this malloc()?

Thanks in advance,
  rdiez

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[Jeff Johnston]

The code does not disable via NDEBUG because it is a fix for a CVE.  It is
not (and should not be) tied to user control over usage of the assert macro.

If you do not want the verification check, you can use
--disable-newlib-reent-check-verify when you configure.

-- Jeff J.

On Mon, Apr 27, 2020 at 11:58 AM R. Diez via Newlib <newlib@sourceware.org>
wrote:

> Hallo all:
>
> I have used Newlib 3.1.0 (and older versions) for quite some time. I am
> trying to upgrade to the latest release/snapshot 3.3.0, but I hit a
> compilation error in release builds (with NDEBUG defined):
>
> newlib-3.3.0/newlib/libc/stdlib/rand.c:78: undefined reference to
> `__assert_func'
>
> I tracked it down to this definition:
>
> /* Specify how to handle reent_check malloc failures. */
> #ifdef _REENT_CHECK_VERIFY
> #include <assert.h>
> #define __reent_assert(x) ((x) ? (void)0 : __assert_func(__FILE__,
> __LINE__, (char *)0, "REENT malloc succeeded"))
> #else
> #define __reent_assert(x) ((void)0)
> #endif
>
> This is unfortunate. First of all, I wonder what happens if malloc fails
> and there is no assert. Will there be a crash?
>
> Then, I would like to assert() in debug builds, and not in release builds.
> My code does not define __assert_func in release builds, because assertions
> are only supposed to work if NDEBUG is not defined. That has been working
> fine for years, until this Newlib version.
>
> I am configuring Newlib with --disable-newlib-multithread , because my
> embedded firmware has no threads. But I guess I still have to deal with
> "struct _reent", don't I? I would have hoped that, in this single-thread
> situation, any reentrancy structure could be allocated statically. Or is
> there any way to avoid this malloc()?
>
> Thanks in advance,
>   rdiez
>
>

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

> The code does not disable via NDEBUG because it is a fix for a CVE.
> It is not (and should not be) tied to user control over usage of the assert macro.
> [...]

First of all, thanks for you quick answer.

I guess you mean CVE-2019-14871.

The "fix" for this CVE feels wrong. It seems that you are trading accessing a NULL pointer with a total firmware crash. I believe that there is no other way that __assert_func() could behave to "fix" this problem. Well, that is trading a security problem for a denial of service problem. This is not really properly fixing the problem.

Firstly, is there no other routine to abort the firmware? __assert_func() should only be used together with assert(). Is it documented anywhere that __assert_func() must stop execution in order to prevent a security hole?

Is there a way to avoid malloc() at all at a place where the user does not expect for it to happen? For example, preallocating all memory that might be needed. If may be worth the trade-off space vs safety.

Like I said, my firmware does not use threads at all. Is there a way to drop all these reentrancy stuff? I am already using --disable-newlib-multithread .

In any case, I though the assertion message "REENT malloc succeeded" is wrong, it should probably read "REENT malloc failed". Or am I reading the code wrong?

Thanks again,
  rdiez

 newlib-3.3.0/newlib/libc/stdlib/rand.c:78: undefined reference to `__assert_func'
> 
> I tracked it down to this definition:
> 
> /* Specify how to handle reent_check malloc failures. */
> #ifdef _REENT_CHECK_VERIFY
> #include <assert.h>
> #define __reent_assert(x) ((x) ? (void)0 : __assert_func(__FILE__, __LINE__, (char *)0, "REENT malloc succeeded"))
> #else
> #define __reent_assert(x) ((void)0)
> #endif
> 
> This is unfortunate. First of all, I wonder what happens if malloc fails and there is no assert. Will there be a crash?
> 
> Then, I would like to assert() in debug builds, and not in release builds. My code does not define __assert_func in release builds, because assertions are only supposed to work if NDEBUG is not defined. That has been working fine for years, until this Newlib version.
> 
> I am configuring Newlib with --disable-newlib-multithread , because my embedded firmware has no threads. But I guess I still have to deal with "struct _reent", don't I? I would have hoped that, in this single-thread situation, any reentrancy structure could be allocated statically. Or is there any way to avoid this malloc()?
> 
> Thanks in advance,
>   rdiez
> 
> 

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[Jeff Johnston]

On Mon, Apr 27, 2020 at 4:29 PM R. Diez <rdiezmail-newlib@yahoo.de> wrote:

> > The code does not disable via NDEBUG because it is a fix for a CVE.
> > It is not (and should not be) tied to user control over usage of the
> assert macro.
> > [...]
>
> First of all, thanks for you quick answer.
>
> I guess you mean CVE-2019-14871.
>
> The "fix" for this CVE feels wrong. It seems that you are trading
> accessing a NULL pointer with a total firmware crash. I believe that there
> is no other way that __assert_func() could behave to "fix" this problem.
> Well, that is trading a security problem for a denial of service problem.
> This is not really properly fixing the problem.
>

An alternative change would require modifications to all the existing
conversion routines using eBalloc() and their callers to do checking of
return values and bubble up to the user, setting errno to ENOMEM.  As no
one had an issue with the null pointer exception prior to the CVE it wasn't
felt that many, if any, people were running into this issue.  It should be
noted that Balloc() storage gets reused once allocated (i.e. is not
freed/allocated again).

>
> Firstly, is there no other routine to abort the firmware? __assert_func()
> should only be used together with assert(). Is it documented anywhere that
> __assert_func() must stop execution in order to prevent a security hole?
>
> Is there a way to avoid malloc() at all at a place where the user does not
> expect for it to happen? For example, preallocating all memory that might
> be needed. If may be worth the trade-off space vs safety.
>
>
The memory in question is being allocated by Balloc() which is part of the
mprec.c solution used in newlib.  The allocated _REENT_MP_FREELIST has an
array of storage to reuse for different k values so newlib will reuse
the storage over and over.  You could conceivably pre-populate this list by
doing sample stdlib conversion calls at the beginning of your application.
Other storage needs requires a bit of estimation on your part and
examination for
memory leakage in your application.  This goes without saying.

Like I said, my firmware does not use threads at all. Is there a way to
> drop all these reentrancy stuff? I am already using
> --disable-newlib-multithread .
>
>
Reentrancy is part and parcel of the newlib code base so that it "can"
support multiple threads.  Most of what you wlil need is allocated
initially in impure.c.  This excludes dynamic storage such as from files
being opened
and the Balloc storage used by the stdlib conversion routines.  If you are
constantly finding yourself close to the threshold, you should either have
a larger memory store to begin with or you need to evaulate how your
application is using memory.

In any case, I though the assertion message "REENT malloc succeeded" is
> wrong, it should probably read "REENT malloc failed". Or am I reading the
> code wrong?
>

The code forms a message: Assertion "xxxx" failed: ....   where xxxx is the
thing you are asserting to be true.  In this case, we wish to assert that
the REENT malloc succeeded.


> Thanks again,
>   rdiez
>
>  newlib-3.3.0/newlib/libc/stdlib/rand.c:78: undefined reference to
> `__assert_func'
> >
> > I tracked it down to this definition:
> >
> > /* Specify how to handle reent_check malloc failures. */
> > #ifdef _REENT_CHECK_VERIFY
> > #include <assert.h>
> > #define __reent_assert(x) ((x) ? (void)0 : __assert_func(__FILE__,
> __LINE__, (char *)0, "REENT malloc succeeded"))
> > #else
> > #define __reent_assert(x) ((void)0)
> > #endif
> >
> > This is unfortunate. First of all, I wonder what happens if malloc fails
> and there is no assert. Will there be a crash?
> >
> > Then, I would like to assert() in debug builds, and not in release
> builds. My code does not define __assert_func in release builds, because
> assertions are only supposed to work if NDEBUG is not defined. That has
> been working fine for years, until this Newlib version.
> >
> > I am configuring Newlib with --disable-newlib-multithread , because my
> embedded firmware has no threads. But I guess I still have to deal with
> "struct _reent", don't I? I would have hoped that, in this single-thread
> situation, any reentrancy structure could be allocated statically. Or is
> there any way to avoid this malloc()?
> >
> > Thanks in advance,
> >   rdiez
> >
> >
>
>

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

> An alternative change would require modifications to all the
> existing conversion routines using eBalloc() and their callers
> to do checking of return values and bubble up to the user,
> setting errno to ENOMEM.

I am not sure what conversion routines you are referring to. I found this issue with rand(), because lwIP needs a source of random numbers. It is normally not expected that rand() calls malloc(). rand() returns no error indication, according to the POSIX standard:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/rand.html

So using errno here is difficult.

rand() ends up calling _REENT_CHECK, which does a malloc(). What do you mean with eBalloc()?

  
> The memory in question is being allocated by Balloc()
> which is part of the mprec.c solution used in newlib.
> The allocated _REENT_MP_FREELIST has an array
> of storage to reuse for different k values so newlib will reuse
> [...]

Does that apply in my scenario? I am building Newlib without thread support. There is no reent creation or destruction, as far as I can tell.

I have been digging further, and I believe (the code is not easy to follow) that these unexpected allocations come from using the "small reent". What does --enable-newlib-reent-small actually do? The README file at this location:

  https://sourceware.org/newlib/README

has this description:

  `--enable-newlib-reent-small'
    Enable small reentrant struct support.
    Disabled by default.

But it does not really say what the difference between the normal and "small" versions is.

Thanks in advance,
  rdiez

FAQ link to crossgcc broken

[R. Diez]

Hallo all:

I wanted  to point out that, on this page:

  https://sourceware.org/newlib/faq.html

under question 3:

  I want to have newlib built with gcc and other components
  from the net. How do I do this?

the following link is broken:

  http://sthoward.dyndns.org/CrossGCC/

This is important because many Newlib users have to build their own cross-compilation toolchain.

Best regards,
  rdiez

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

Hallo again:

After some further research and testing, I wanted to share a couple of hints that may help other people too:

Point 1) Configuring with --disable-newlib-reent-small (which is actually the default) gets rid of the surprising malloc() on first touch from rand(), and probably many other places.

Note that sizeof( struct _reent  ) increases from 240 bytes to 1064 bytes (tested with Newlib version 3.3.0).

I think Newlib should document that option --enable-newlib-reent-small is risky. Unless you are really tight on RAM, I strongly advise against this unsafe optimisation.


Point 2) The documentation on this section is misleading:

  https://sourceware.org/newlib/libc.html#Reentrancy

That section states:

-----------8<-----------8<-----------8<-----------8<-----------
This means that you have two ways to achieve reentrancy. Both require that each thread of execution control initialize a unique global variable of type ‘struct _reent’:

Use the reentrant versions of the library functions, after initializing a global reentrancy structure for 1.  each process. Use the pointer to this structure as the extra argument for all library functions.
-----------8<-----------8<-----------8<-----------8<-----------

This section gives you the impression that you are free to choose. But if you have multithreading, you really need to implement _impure_ptr, mention in option (2). For example, the rand() function needs such a global state (that was my impression by looking at the convoluted macros).

Newlib does have a rand_r() counterpart, but it is not really equivalent (it is a much weaker pseudo-random generator), and Posix has declared it obsolete. So there is no real xxx_r() equivalent for rand() in Newlib (that I could see).

All the best,
  rdiez

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

"R. Diez via Newlib" <newlib@sourceware.org> writes:

> Note that sizeof( struct _reent  ) increases from 240 bytes to 1064
> bytes (tested with Newlib version 3.3.0).

If you're tight on RAM, you might look at the newlib fork, picolibc,
which uses native TLS support for reentrant data values like
_rand_next. With that approach, your thread-local storage contains only
values used by your application and malloc need never be called. In this
case, the per-thread data would be only eight bytes.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

> If you're tight on RAM, you might look at the newlib fork, picolibc,
> which uses native TLS support for reentrant data values like
> _rand_next. With that approach, your thread-local storage contains only
> values used by your application and malloc need never be called. In this
> case, the per-thread data would be only eight bytes.

I do have one microcontroller with 8 KiB SRAM. For all others I do not want to bother.

I am interested in your "only eight bytes" claim. Say I use picolibc, and then my firmware wants to use rand(), which needs many more bytes for the 
state of the pseudo-random number generator.

The firmware is bare metal (no operating system). I guess there will not be a native TLS support then.

How does rand() get the memory it needs? Do I need to preallocate it statically? Or have a separate memory pool for the TLS? Do I need to implement 
TLS myself? Can you do that with some sort of easy-to-code stack?

Regards,
   rdiez

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[Jeff Johnston]

On Tue, Apr 28, 2020 at 8:06 AM R. Diez <rdiezmail-newlib@yahoo.de> wrote:

> > An alternative change would require modifications to all the
> > existing conversion routines using eBalloc() and their callers
> > to do checking of return values and bubble up to the user,
> > setting errno to ENOMEM.
>
> I am not sure what conversion routines you are referring to. I found this
> issue with rand(), because lwIP needs a source of random numbers. It is
> normally not expected that rand() calls malloc(). rand() returns no error
> indication, according to the POSIX standard:
>
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/rand.html
>
> So using errno here is difficult.
>
> rand() ends up calling _REENT_CHECK, which does a malloc(). What do you
> mean with eBalloc()?
>

Sorry, my bad for the confusion.  eBalloc() was one part of the CVE whereby
newlib was allocating memory internally for the conversion routines
and not checking the result (this code originally came from NetBSD
originally and assumed the Balloc() would succeed or a NULL pointer access
would occur).  To make this
return gracefully to the user without an abort would require a number of
changes to intermediate routines.

Regarding the _REENT_CHECK_VERIFY macro, this only affects you if you are
using _REENT_SMALL.  With that set-up, the reentrant structure is minimal
to start with and
storage is allocated at the last minute, if needed.  If you look in
libc/sys/reent.h you will see that for the non _REENT_SMALL configuration,
the storage is allocated up front and
the _REENT_CHECK macros do nothing.

In your case, rand() calls the _REENT_CHECK_RAND48() macro which for
_REENT_SMALL will end up allocating the rand48 storage if not already
allocated.

So, you have a number of choices:

1. don't use _REENT_SMALL and rand() won't allocate storage when used
2. use _REENT_SMALL but call rand() at the start of your application so it
won't try to allocate memory later on
3. use _REENT_SMALL and ensure you have enough extra storage allocated to
handle the apps needs and the minimal storage needed for the rand48 logic
4. allow the abort or null pointer access to occur and tune the application
not to run into this scenario if it does happen


>
> > The memory in question is being allocated by Balloc()
> > which is part of the mprec.c solution used in newlib.
> > The allocated _REENT_MP_FREELIST has an array
> > of storage to reuse for different k values so newlib will reuse
> > [...]
>
> Does that apply in my scenario? I am building Newlib without thread
> support. There is no reent creation or destruction, as far as I can tell.
>

If you aren't directly/indirectly using the mprec routines in stdlib, no,
it does not apply to you.  A reent structure is always allocated (see
impure_data in libc/reent/impure.c).

>
> I have been digging further, and I believe (the code is not easy to
> follow) that these unexpected allocations come from using the "small
> reent". What does --enable-newlib-reent-small actually do? The README file
> at this location:
>
>   https://sourceware.org/newlib/README
>
> has this description:
>
>   `--enable-newlib-reent-small'
>     Enable small reentrant struct support.
>     Disabled by default.
>
> But it does not really say what the difference between the normal and
> "small" versions is.
>

The difference is that _REENT_SMALL omits a bunch of reentrant storage used
by various routines until they are actually used.  Thus, your reentrant
structure
footprint is the minimum it can be for your application (e.g. if you never
call rand(), you don't need the rand48 structure).  If you never do file
I/O, you don't
need the std streams allocated for you and so on.  The cost of this is that
if your application has run out of memory, you might not have enough for
some of
these last second allocations.  The CVE in question noted that newlib
simply attempted to access the reentrant allocated storage without checking
and it could be null.  That said, if you don't have enough storage
for a small reentrant structure need, the application is in serious trouble
to continue because many platforms share storage between the stack and the
heap (you might not have
enough storage for another call).  I wouldn't have a lot of faith that an
application will check after every call it makes to see if ENOMEM is set
and then magically remove some storage it didn't really need.
Such a case usually means either tinkering with the initial storage
available or tuning the application so it isn't wasting storage needlessly.

Regards,

-- Jeff J.


> Thanks in advance,
>   rdiez
>
>

Re: _REENT_CHECK_VERIFY calls __assert_func even if NDEBUG is defined

[R. Diez]

 > [...]
> Regarding the _REENT_CHECK_VERIFY macro, this only affects you if you are using _REENT_SMALL.

I have been looking around further.

I have come to the conclusion that advertising configuration option "--enable-newlib-reent-small" as "enable small reentrant struct support" is 
actually doing a disservice to Newlib's users, especially in an embedded environment.

Those surprising calls to malloc() have a performance impact, as malloc() can be slow. They also cause safety issues, because of the unexpected extra 
memory consumption (memory is sometimes painstakingly accounted for). After all, there has already been a CVE in this area, and memory exhaustion at 
this unexpected place will crash the firmware. There is a further issue: malloc() is not safe everywhere (like inside an interrupt routine).

Therefore, I believe the only right thing to do is to place a warning next to that configuration option. For example, something like "warning: causes 
hidden malloc()'s on first touch" should suffice.


> With that set-up, the reentrant structure is minimal to start with and
> storage is allocated at the last minute, if needed.  If you look in libc/sys/reent.h
 > you will see that for the non _REENT_SMALL configuration, the
 > [...]

OK, thanks for confirming this.

There is no libc/sys/reent.h as far as I can tell. Confusingly, there are 2 files with the same name in Newlib:

newlib/libc/include/reent.h
newlib/libc/include/sys/reent.h

I guess you are referring to the second one. For convenience, it's here:

https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/reent.h

I looked inside, and I do not understand why struct _reent has to be 1064 bytes long. There is space for __FILE structures, but my firmware is not 
using the STDIO routines at all. I am configuring with --disable-newlib-io-float, --disable-newlib-iconv , etc., so my guess is that many of the 
members inside struct _reent will never be used and could be optimised away with #if statements.

You could even configure the rand() data away if you can live with the weaker rand_r() variant.

There are some comments there about trying to maintain binary compatibility. Is this something that Cygwin needs? At least in an embedded environment 
there is no need for that. I thought that struct _reent is something internal to Newlib, or are there public fields meant for the library user?

Note that the size of this structure is not always the same. For example, member "_atexit" depends on "#ifndef _REENT_GLOBAL_ATEXIT". Therefore, this 
structure already varies in length today.


> So, you have a number of choices:
> 
> 1. don't use _REENT_SMALL and rand() won't allocate storage when used
> 2. use _REENT_SMALL but call rand() at the start of your application so it won't try to allocate memory later on
> 3. use _REENT_SMALL and ensure you have enough extra storage allocated to handle the apps needs and the minimal storage needed for the rand48 logic
> 4. allow the abort or null pointer access to occur and tune the application not to run into this scenario if it does happen

This is an excellent summary about dealing with configuration option "--enable-newlib-reent-small". Is there any chance it could make it to the Newlib 
documentation?

Best regards,
   rdiez