Re: Usage of __assert_func in standard library

[Jeff Johnston <jjohnstn@redhat.com>]

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

On Tue, Nov 21, 2023 at 5:05 AM Alex Tarasov <tarasov.alex.m.tr@gmail.com>
wrote:

> On Tue, Nov 21, 2023 at 0:56 AM Jeff Johnston <jjohnstn@redhat.com> wrote:
>
>>
>> On Mon, Nov 20, 2023 at 5:34 AM Corinna Vinschen <vinschen@redhat.com>
>> wrote:
>>
>>> On Nov 18 19:13, Alex Tarasov wrote:
>>> > Dear developers of  Newlib,
>>> >
>>> > I think we have a bit of an issue concerning usage of *__assert_func
>>> *function
>>> > in the current version of the standard library.
>>> > [...]
>>> > ld.exe:
>>> PathToCompiler/../lib/gcc/arm-none-eabi/13.2.1/thumb/v7e-m+fp/hard\libg.a(libc_a-closer.o):
>>> > in function `_close_r':
>>> > closer.c:(.text._close_r+0xc): undefined reference to `_close'
>>> > [...]
>>> > The analysis of cross reference table in the .map file showed me the
>>> source
>>> > of these errors. Somehow, in the newer toolchain functions from the
>>> > standard library (*strtod* in my case) call *"__assert_func"* which in
>>> turn
>>> > lead to various system calls. I downloaded the latest Newlib version
>>> on the
>>> > "main" branch and saw this code in the *newlib/libc/stdlib/mprec.h*
>>> file:
>>> >
>>> > #define eBalloc(__reent_ptr, __len) ({ \
>>> >    void *__ptr = Balloc(__reent_ptr, __len); \
>>> >    if (__ptr == NULL) \
>>> >      __assert_func(__FILE__, __LINE__, (char *)0, "Balloc succeeded");
>>> \
>>> >    __ptr; \
>>> >    })
>>> >
>>> > According to *git log* this *eBalloc* macro was introduced in commit
>>> > with f88aece242178ff0c187d56e34a79645fbc44a23
>>> > hash on October 4th 2019. This leads to several functions in the
>>> standard
>>> > library calling *__assert_func* inside them. Standard definition of
>>> this
>>> > function in *newlib/libc/stdlib/assert.c* calls *abort* and *fiprintf*
>>> > functions
>>> > which in turn drag a lot of system functions (see error log that I
>>> > mentioned above).
>>> >
>>> > This leads to several issues:
>>> >
>>> >    - If I want to use some functions from the standard library (like
>>> >    *strtod*) but I don't want any system calls, I need to redefine
>>> >    *__assert_func* in my project. I have to do it even if I don't use
>>> >    assert's anywhere in my own code.
>>> >    - There is a project where I use <assert.h> and my own
>>> > implementation of *__assert_func.
>>> >    *I expect that when I build my project with NDEBUG macro defined
>>> >    (release configuration), I would not see any calls to that
>>> function. That
>>> >    is not the case if any function from Newlib's standard library that
>>> uses
>>> >    *eBalloc* macro gets linked.
>>> >
>>> > I think this behaviour is too implicit and needs to be fixed. I suggest
>>> > removing *__assert_func* from *eBalloc* macro or making some other
>>> changes
>>> > to Newlib that will get rid of the mentioned issues.
>>>
>>> Yeah, that looks a tad too aggressive.
>>>
>>> I see only ~20 calls to eBalloc in newlib.  Maybe somebody can take
>>> a stab at it and convert the calls to Balloc plus error checking?
>>>
>>> However, the parent functions of the functions calling eBalloc
>>> potentially don't check error conditions either, so this might be indeed
>>> a bit more work than a quick count of the eBalloc calls indicates...
>>>
>>> Jeff? Any idea how to go forward?
>>>
>>>
>>> The eBalloc macro calling __assert_func is a fix for a CVE whereby a
>> write to 0 could
>> occur silently within newlib.  It is intentional that it aborts and
>> doesn't disable with NDEBUG.
>>
>> As mentioned by Corinna, the call chain of eBalloc isn't checking results
>> and it was why we
>> chose to use the assert to handle the CVE.
>>
>> That said, one issue for you seems to be the fiprintf() call.  A possible
>> solution is to have
>> a macro which disables the fiprintf() or you can choose to override the
>> fiprintf() method
>> yourself.
>>
>> -- Jeff J.
>>
>>
>> Corinna
>>>
>>>
> Thank you both for your response!
>
> > issue for you seems to be the fiprintf() call
> No, the issue for me is the whole __assert_func() with both fiprintf() and
> abort(). The latter one needs _kill() and _get_pid(), while fiprintf()
> references _write(), _close(), _lseek(), etc.
>
> Try to see it from the perspective of programmers who write new bare-metal
> projects. Let's assume that they need to parse string-messages from some
> device. They decide to use the strtod() function from the standard library
> to get some values from these messages. Suddenly, they get errors from the
> linker about missing definitions of functions _kill(), _write(), _close()
> etc. To resolve this issue they need to check the .map file and try to
> untangle the references from the Cross Reference Table. Which is not an
> easy task since you can only see what object file contains a call to your
> function. Only then can a programmer see that the reason for their linkage
> error are calls to __assert_func() which in turn leads to other system
> calls. Then they have to consider what to do with this. The best approach
> would be (as far as I know) to download Newlib, look at where this
> __assert_func() is called, see that it is only needed to check Balloc's
> result, and then implement your own definition of __assert_func() that
> doesn't lead to any system calls (that's what I've done).
>
> In my opinion this is far too much for simply trying to use a strtod()
> function from the standard library. Using -specs=nosys.specs here can also
> do the trick, but I don't consider it a good solution since it leaves a lot
> of unnecessary stub functions. It also makes it less clear what is being
> linked in your program. So, it's either spending a lot of time debugging
> the issue or using less effective approaches like implementing different
> linkage flags or avoiding standard library at all.
>
> I know that in a new project when using the standard library you also have
> to deal with dynamic memory allocation. You have to provide a
> definition for sbrk() or redefine malloc_r(), free_r(), etc yourself. But
> this seems to be common knowledge, while the case with __assert_func() is
> not.
>
> As for the reasons for this __assert_func(), wasn't it just a quick fix?
> Isn't it a better solution to check for NULL everywhere Balloc get's called
> (as Corrina already suggested)? I know that it can be a lot of work since
> we need to untangle a lot of calls but it can also save a lot of time for
> the users of Newlib.
>
> -- Alexander T.
>

It is not just a quick and dirty fix.  The application has run out of
memory deep in the newlib internals.  There are methods that don't have
failure return codes and/or are not documented to set an errno for out of
memory.   It is not reasonable that the user should be required to add code
to try to recognize such a situation.  That said, I think that newlib
aborting without the fiprintf is quite reasonable.  Your complaint that
_kill() and _get_pid() being required is not a real issue considering these
are part of the base syscall set for newlib and there is a libnosys()
library to provide stubbed calls if they aren't implemented for the
particular BSP or the user doesn't want a full-fledged implementation.

-- Jeff J.