On Tue, Mar 28, 2023 at 04:28:58PM -0400, Nathan Hartman wrote:
> On Tue, Mar 28, 2023 at 3:43 PM Fotis Panagiotopoulos
> <f.j.pa...@gmail.com> wrote:
> >
> > Hello,
>
> Replying inline below...
>
> > I just noticed that there are some problems with the usage of asprintf()
> > throughout the code base.
> > This function is not properly checked for failure, which can lead to nasty
> > crashes.
> >
> > I am checking here: https://linux.die.net/man/3/asprintf
> > It states:
> >
> > > Return Value When successful, these functions return the number of bytes
> > > printed, just like *sprintf <https://linux.die.net/man/3/sprintf>(3)*. If
> > > memory allocation wasn't possible, or some other error occurs, these
> > > functions will return -1, and the contents of *strp* is undefined.
> > >
> >
> > Notice that in case of failure, the contents of strp will be undefined.
> > To my knowledge, the only proper way to check for failure is to check for a
> > negative return value.
> >
> > Indeed the NuttX version of this function follows this description.
> > It will always return -1 in case of error. The provided pointer will not be
> > touched, unless the function has succeeded.
> >
> > However, in several cases, the users of asprintf make the assumption that
> > the pointer will be set to NULL in case of failure!
> > (Which is definitely not the case!)
> >
> > For example see libs/libc/stdio/lib_tempnam.c.
> > In case of any error (malloc failure for example), the function will
> > continue with a garbage value in variable `template`.
> > (Neither the variable is initialized, nor the return value of asprintf is
> > checked).
>
> Good catch!
>
> > There are many more such cases throughout NuttX.
> >
> > So, this must be fixed. This serious flaw affects several parts of NuttX,
> > including the standard library, the file system and maybe more.
> >
> > The question is how?
> >
> > The straightforward method would be to search for all uses of asprintf()
> > and refactor them to properly check the return value.
> > I consider this the most "correct" and portable approach, but it is quite
> > some work.
>
> IMO this is the correct approach and it is the approach we should take.
>From the FreeBSD manpage:
The asprintf() and vasprintf() functions set *ret to be a pointer to a
buffer sufficiently large to hold the formatted string. This pointer
should be passed to free(3) to release the allocated storage when it is
no longer needed. If sufficient space cannot be allocated, asprintf()
and vasprintf() will return -1 and set ret to be a NULL pointer.
However, NetBSD says:
asprintf() and vasprintf() set the ret argument to a pointer containing
the formatted string. This pointer points to a newly allocated buffer
and should be passed to free(3) to release the allocated storage when it
is no longer needed. If sufficient space cannot be allocated, these
functions will return -1 and set ret to be a NULL pointer. Please note
that these functions are not standardized, and not all implementations
can be assumed to set the ret argument to NULL on error. It is more por-
table to check for a return value of -1 instead.
I think I should open up a ticket for FreeBSD to extend this part in the
manpage.
Sounds like a trap - I often just look into the FreeBSD manpages, since this
is what my desktop runs.
It seems that the GNU manpage declares the pointer undefined, although it
might still set it to NULL.
In my opinion asprintf should set the pointer to NULL, to be safe.
But the calling code should probably be changed as well, because it is
not a good coding example for portability.
>
> Since, as you point out, this may be a lot of work, I think we should
> try to split the work across several devs...
>
> To do that, we'd grep for all uses of asprintf() to find out which
> files use it and post that list in reply to this email.
>
> Hopefully multiple people will volunteer to choose some block of
> filenames from that list to inspect the call sites of asprintf() in
> those files and fix the incorrect ones.
>
> > Another idea is to change asprintf() to set the pointer to NULL in case of
> > failure.
> > We decide that this is how the NuttX's version of asprintf() works, and we
> > satisfy the assumption of the callers.
> > I think that this will instantly solve all problems.
>
> You could, but... Doing that will not fix the call sites, so it is not
> future-proof. The call sites are incorrect and really should be fixed.
> If we ever swap out the implementation of asprintf() for a different
> one, etc., the problems would return unless we fix the call sites.
>
> > FInally, we can just make sure that the provided pointer is always
> > initialized to NULL.
> > Since it will not be touched in case of failure, we can move on with
> > minimal effort.
> > I am not sure about portability though...
>
> Unless strp itself is NULL, in which case that won't work.
>
> Cheers,
> Nathan
--
B.Walter <be...@bwct.de> https://www.bwct.de
Modbus/TCP Ethernet I/O Baugruppen, ARM basierte FreeBSD Rechner uvm.
