You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Fotis Panagiotopoulos <f....@gmail.com> on 2023/03/28 19:43:18 UTC
asprintf failure
Hello,
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).
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.
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.
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...
What do you think?
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
Indeed your suggestion is valid.
I created the following issues:
https://github.com/apache/nuttx/issues/9126
https://github.com/apache/nuttx-apps/issues/1727
On Thu, Apr 27, 2023 at 5:06 AM Nathan Hartman <ha...@gmail.com>
wrote:
> On Wed, Apr 26, 2023 at 9:32 AM Fotis Panagiotopoulos
> <f....@gmail.com> wrote:
> >
> > Hello,
> >
> > I fixed all critical usages of asprintf and strdup.
> >
> > These fixes are in the following PRs:
> > https://github.com/apache/nuttx/pull/9009
> > https://github.com/apache/nuttx/pull/9010
> > https://github.com/apache/nuttx-apps/pull/1713
> >
> >
> > The OS itself is now fixed, there should be no other issues pending.
> >
> > However, there are some uses that are still to be fixed.
> > These are in the tools directory and in some apps (strdup mostly).
> >
> > They should not affect the stability of the system in any way, and I
> > consider these "minor" issues.
> >
> > Unfortunately, I cannot spend any more time on this.
> > So, if anyone has any free time, I would encourage you to improve the
> apps
> > or the tools in this regard.
>
> Thank you for doing this!
>
> If I may make a suggestion, it would be helpful to post an issue to
> the issue tracker about the ones that aren't fixed yet (strdup etc) so
> that we won't forget to eventually fix them.
>
> Thanks again for taking care of these!!
>
> Nathan
>
Re: asprintf failure
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Apr 26, 2023 at 9:32 AM Fotis Panagiotopoulos
<f....@gmail.com> wrote:
>
> Hello,
>
> I fixed all critical usages of asprintf and strdup.
>
> These fixes are in the following PRs:
> https://github.com/apache/nuttx/pull/9009
> https://github.com/apache/nuttx/pull/9010
> https://github.com/apache/nuttx-apps/pull/1713
>
>
> The OS itself is now fixed, there should be no other issues pending.
>
> However, there are some uses that are still to be fixed.
> These are in the tools directory and in some apps (strdup mostly).
>
> They should not affect the stability of the system in any way, and I
> consider these "minor" issues.
>
> Unfortunately, I cannot spend any more time on this.
> So, if anyone has any free time, I would encourage you to improve the apps
> or the tools in this regard.
Thank you for doing this!
If I may make a suggestion, it would be helpful to post an issue to
the issue tracker about the ones that aren't fixed yet (strdup etc) so
that we won't forget to eventually fix them.
Thanks again for taking care of these!!
Nathan
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
Hello,
I fixed all critical usages of asprintf and strdup.
These fixes are in the following PRs:
https://github.com/apache/nuttx/pull/9009
https://github.com/apache/nuttx/pull/9010
https://github.com/apache/nuttx-apps/pull/1713
The OS itself is now fixed, there should be no other issues pending.
However, there are some uses that are still to be fixed.
These are in the tools directory and in some apps (strdup mostly).
They should not affect the stability of the system in any way, and I
consider these "minor" issues.
Unfortunately, I cannot spend any more time on this.
So, if anyone has any free time, I would encourage you to improve the apps
or the tools in this regard.
On Wed, Mar 29, 2023 at 8:07 PM Nathan Hartman <ha...@gmail.com>
wrote:
> On Wed, Mar 29, 2023 at 5:02 AM Fotis Panagiotopoulos <f.j.panag@gmail.com
> >
> wrote:
>
> > > 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.
> >
> > I'm sceptical about this.
> > Setting the pointer to NULL seems more safe, but also it is a change in
> > functionality!
> >
> > Consider the following example:
> >
> > char * msg = "Error message";
> > asprintf(&msg, "format string", args...);
> >
> > Based on the current functionality, I can directly use msg without any
> > error checking, as it will always be valid.
> > (Either due to its initialization, or due to a successful asprintf).
> >
> > Indeed, this seems like a not-so-great piece of code, but I don't know
> > whether this approach is used anywhere in NuttX
> > (or in user code).
> >
>
>
> The above usage may work on some implementations but not others. As Bernd
> Walter pointed out from some *BSD manpages, some implementations will set
> your msg pointer to NULL, losing your fallback string. So code that runs
> perfectly well on one OS may break when run on another.
>
> If you have many usages like that, you could encapsulate the
> asprintf-or-default-string functionality in a function you can write for
> that purpose. You'd utilize vararg and vasprintf for it.
>
> Cheers
> Nathan
>
Re: asprintf failure
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Mar 29, 2023 at 5:02 AM Fotis Panagiotopoulos <f....@gmail.com>
wrote:
> > 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.
>
> I'm sceptical about this.
> Setting the pointer to NULL seems more safe, but also it is a change in
> functionality!
>
> Consider the following example:
>
> char * msg = "Error message";
> asprintf(&msg, "format string", args...);
>
> Based on the current functionality, I can directly use msg without any
> error checking, as it will always be valid.
> (Either due to its initialization, or due to a successful asprintf).
>
> Indeed, this seems like a not-so-great piece of code, but I don't know
> whether this approach is used anywhere in NuttX
> (or in user code).
>
The above usage may work on some implementations but not others. As Bernd
Walter pointed out from some *BSD manpages, some implementations will set
your msg pointer to NULL, losing your fallback string. So code that runs
perfectly well on one OS may break when run on another.
If you have many usages like that, you could encapsulate the
asprintf-or-default-string functionality in a function you can write for
that purpose. You'd utilize vararg and vasprintf for it.
Cheers
Nathan
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
strdup() is much better used. Its functionality is simpler, and its
behavior much better understood by most people.
There are only two places with a strdup without checking the result:
gs2200m.c, line 3503
nxffs_pack.c, line 1092
Either the original author forgot it, or it doesn't matter?
There are also some cases in the `tools` directory, but these are less
important.
The worst thing that can happen is the build to fail, not having a system
crash...
On Wed, Mar 29, 2023 at 6:34 PM Fotis Panagiotopoulos <f....@gmail.com>
wrote:
> > strdup?
>
> Thanks, I will check this too.
>
> I went through all the asprintf calls in the list.
> These are the ones that actually need to be fixed.
> Everything else is properly checked.
>
> drivers/net/telnet.c
> 698: ret = asprintf(&devpath, TELNET_DEVFMT, priv->td_minor);
>
> libs/libc/uuid/lib_uuid_to_string.c
> 66: c = asprintf(s,
>
> libs/libc/stdio/lib_tempnam.c
> 78: asprintf(&template, "%s/%s-XXXXXX", dir, pfx);
>
> tools/kconfig2html.c
> 2295: asprintf(&dirpath, "%s/%s%s%s",
> 2300: asprintf(&dirpath, "%s/%s",
> g_kconfigroot, subdir);
> 2435: asprintf(&kconfigpath, "%s/%s", kconfigdir, kconfigname);
>
> tools/gencromfs.c
> 1114: ret = asprintf(&path, "%s/%s", dirpath, name);
> 1144: ret = asprintf(&path, "%s/%s", dirpath, name);
>
> fs/vfs/fs_dir.c
> 603: asprintf(&dir->fd_path, "%s%s/", path_prefix, relpath);
>
> fs/vfs/fs_rename.c
> 131: asprintf(&subdir, "%s/%s", newpath, subdirname);
> 372: asprintf(&subdir, "%s/%s", newrelpath,
>
> fs/inode/fs_inodesearch.c
> 356: asprintf(&buffer,
> 484: asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
>
>
> Specifically the telnet.c one, does check the return code, but it proceeds
> nevertheless instead of aborting.
> The function ends up doing half of the job it was supposed to do. Is this
> OK here?
>
> Specifically the call in lib_uuid_to_string, indeed checks the result.
> But I cannot understand what needs to be done to `s`.
> What does the standard specify? Shall we set it to NULL in case of error?
> Or is it undefined?
>
>
> On Wed, Mar 29, 2023 at 4:12 PM Gregory Nutt <sp...@gmail.com> wrote:
>
>>
>> > I can do that. Apart from asprintf() and vasprintf(), is anyone aware of
>> > any other similarly suspicious functions to check?
>>
>> strdup?
>>
>>
>>
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
> strdup?
Thanks, I will check this too.
I went through all the asprintf calls in the list.
These are the ones that actually need to be fixed.
Everything else is properly checked.
drivers/net/telnet.c
698: ret = asprintf(&devpath, TELNET_DEVFMT, priv->td_minor);
libs/libc/uuid/lib_uuid_to_string.c
66: c = asprintf(s,
libs/libc/stdio/lib_tempnam.c
78: asprintf(&template, "%s/%s-XXXXXX", dir, pfx);
tools/kconfig2html.c
2295: asprintf(&dirpath, "%s/%s%s%s",
2300: asprintf(&dirpath, "%s/%s",
g_kconfigroot, subdir);
2435: asprintf(&kconfigpath, "%s/%s", kconfigdir, kconfigname);
tools/gencromfs.c
1114: ret = asprintf(&path, "%s/%s", dirpath, name);
1144: ret = asprintf(&path, "%s/%s", dirpath, name);
fs/vfs/fs_dir.c
603: asprintf(&dir->fd_path, "%s%s/", path_prefix, relpath);
fs/vfs/fs_rename.c
131: asprintf(&subdir, "%s/%s", newpath, subdirname);
372: asprintf(&subdir, "%s/%s", newrelpath,
fs/inode/fs_inodesearch.c
356: asprintf(&buffer,
484: asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
Specifically the telnet.c one, does check the return code, but it proceeds
nevertheless instead of aborting.
The function ends up doing half of the job it was supposed to do. Is this
OK here?
Specifically the call in lib_uuid_to_string, indeed checks the result.
But I cannot understand what needs to be done to `s`.
What does the standard specify? Shall we set it to NULL in case of error?
Or is it undefined?
On Wed, Mar 29, 2023 at 4:12 PM Gregory Nutt <sp...@gmail.com> wrote:
>
> > I can do that. Apart from asprintf() and vasprintf(), is anyone aware of
> > any other similarly suspicious functions to check?
>
> strdup?
>
>
>
Re: asprintf failure
Posted by Gregory Nutt <sp...@gmail.com>.
> I can do that. Apart from asprintf() and vasprintf(), is anyone aware of
> any other similarly suspicious functions to check?
strdup?
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
Using ack, these are all uses of asprintf():
drivers/net/telnet.c
698: ret = asprintf(&devpath, TELNET_DEVFMT, priv->td_minor);
libs/libc/uuid/lib_uuid_to_string.c
66: c = asprintf(s,
libs/libc/stdio/lib_tempnam.c
78: asprintf(&template, "%s/%s-XXXXXX", dir, pfx);
tools/initialconfig.c
465: ret = asprintf(&archpath, "%s%c%s", dirpath, g_delim, entry->d_name);
469: "ERROR: asprintf() failed to archpath\n");
473: ret = asprintf(&testpath, "%s%cKconfig", archpath, g_delim);
477: "ERROR: asprintf() failed to testpath\n");
485: ret = asprintf(&testpath, "%s%cinclude", archpath, g_delim);
489: "ERROR: asprintf() failed to testpath/include\n");
497: ret = asprintf(&testpath, "%s%csrc", archpath, g_delim);
501: "ERROR: asprintf() failed to testpath/src\n");
541: ret = asprintf(&mcupath, "%s%c%s", dirpath, g_delim, entry->d_name);
545: "ERROR: asprintf() failed to mcupath\n");
549: ret = asprintf(&testpath, "%s%cKconfig", mcupath, g_delim);
553: "ERROR: asprintf() failed to archpath/Kconfig\n");
560: ret = asprintf(&testpath, "%s%cMake.defs", mcupath, g_delim);
564: "ERROR: asprintf() failed to testpath/Make.defs\n");
605: ret = asprintf(&configpath, "%s%c%s%cdefconfig",
610: "ERROR: asprintf() failed to configpath\n");
624: ret = asprintf(&varvalue, "\"%s\"", g_selected_mcu);
628: "ERROR: asprintf() failed to varvalue\n");
696: ret = asprintf(&boardpath, "%s%c%s", dirpath, g_delim, entry->d_name);
700: "ERROR: asprintf() failed to boardpath\n");
704: ret = asprintf(&testpath, "%s%cKconfig", boardpath, g_delim);
708: "ERROR: asprintf() failed to testpath\n");
715: ret = asprintf(&testpath, "%s%cinclude", boardpath, g_delim);
719: "ERROR: asprintf() failed to testpath\n");
726: ret = asprintf(&testpath, "%s%csrc", boardpath, g_delim);
730: "ERROR: asprintf() failed to archpath\n");
913: ret = asprintf(&archpath, "%s%c%s%csrc",
918: "ERROR: asprintf() failed to archpath/src\n");
tools/incdir.c
211:static int my_asprintf(char **strp, const char *fmt, ...)
415: ret = my_asprintf(&segment, "%s'%s'", cmdarg,
incpath);
419: ret = my_asprintf(&segment, "%s'%s", cmdarg, incpath);
426: ret = my_asprintf(&segment, ";%s'", incpath);
430: ret = my_asprintf(&segment, ";%s", incpath);
444: ret = my_asprintf(&segment, "%s \"%s\"", cmdarg, incpath);
448: ret = my_asprintf(&segment, " %s \"%s\"", cmdarg,
incpath);
tools/kconfig2html.c
2295: asprintf(&dirpath, "%s/%s%s%s",
2300: asprintf(&dirpath, "%s/%s",
g_kconfigroot, subdir);
2435: asprintf(&kconfigpath, "%s/%s", kconfigdir, kconfigname);
tools/gencromfs.c
1114: ret = asprintf(&path, "%s/%s", dirpath, name);
1144: ret = asprintf(&path, "%s/%s", dirpath, name);
arch/xtensa/src/esp32/esp32_wifi_adapter.c
3006: ret = asprintf(&index_name, "%s", name);
3093: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3168: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3236: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
arch/xtensa/src/esp32s3/esp32s3_wifi_adapter.c
2914: ret = asprintf(&index_name, "%s", name);
3001: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3076: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3144: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
arch/risc-v/src/esp32c3/esp32c3_wifi_adapter.c
3142: ret = asprintf(&index_name, "%s", name);
3229: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3304: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
3372: ret = asprintf(&dir, NVS_DIR_BASE"%s.%s", index_name, key);
fs/unionfs/fs_unionfs.c
717: ret = asprintf(&relpath, "%s%s", path, name);
721: ret = asprintf(&relpath, "%s/%s", path, name);
fs/vfs/fs_dir.c
603: asprintf(&dir->fd_path, "%s%s/", path_prefix, relpath);
fs/vfs/fs_rename.c
131: asprintf(&subdir, "%s/%s", newpath, subdirname);
372: asprintf(&subdir, "%s/%s", newrelpath,
fs/inode/fs_inodesearch.c
356: asprintf(&buffer,
484: asprintf(&desc->buffer, "%s/%s", _inode_getcwd(), desc->path);
On Wed, Mar 29, 2023 at 12:02 PM Fotis Panagiotopoulos <f....@gmail.com>
wrote:
> > Since, as you point out, this may be a lot of work, I think we should
> > try to split the work across several devs...
>
> Yes please, I am beyond burn-out lately...
>
> > 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.
>
> I can do that. Apart from asprintf() and vasprintf(), is anyone aware of
> any other similarly suspicious functions to check?
>
> > 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.
>
> I'm sceptical about this.
> Setting the pointer to NULL seems more safe, but also it is a change in
> functionality!
>
> Consider the following example:
>
> char * msg = "Error message";
> asprintf(&msg, "format string", args...);
>
> Based on the current functionality, I can directly use msg without any
> error checking, as it will always be valid.
> (Either due to its initialization, or due to a successful asprintf).
>
> Indeed, this seems like a not-so-great piece of code, but I don't know
> whether this approach is used anywhere in NuttX
> (or in user code).
>
>
>
> On Wed, Mar 29, 2023 at 3:28 AM Tomek CEDRO <to...@cedro.info> wrote:
>
>> On Wed, Mar 29, 2023 at 2:14 AM Bernd Walter wrote:
>> > 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.
>>
>> greetings from a FBSD station to a fellow daemon :-) :-)
>>
>> --
>> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
>>
>
Re: asprintf failure
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
> Since, as you point out, this may be a lot of work, I think we should
> try to split the work across several devs...
Yes please, I am beyond burn-out lately...
> 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.
I can do that. Apart from asprintf() and vasprintf(), is anyone aware of
any other similarly suspicious functions to check?
> 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.
I'm sceptical about this.
Setting the pointer to NULL seems more safe, but also it is a change in
functionality!
Consider the following example:
char * msg = "Error message";
asprintf(&msg, "format string", args...);
Based on the current functionality, I can directly use msg without any
error checking, as it will always be valid.
(Either due to its initialization, or due to a successful asprintf).
Indeed, this seems like a not-so-great piece of code, but I don't know
whether this approach is used anywhere in NuttX
(or in user code).
On Wed, Mar 29, 2023 at 3:28 AM Tomek CEDRO <to...@cedro.info> wrote:
> On Wed, Mar 29, 2023 at 2:14 AM Bernd Walter wrote:
> > 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.
>
> greetings from a FBSD station to a fellow daemon :-) :-)
>
> --
> CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
>
Re: asprintf failure
Posted by Tomek CEDRO <to...@cedro.info>.
On Wed, Mar 29, 2023 at 2:14 AM Bernd Walter wrote:
> 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.
greetings from a FBSD station to a fellow daemon :-) :-)
--
CeDeROM, SQ7MHZ, http://www.tomek.cedro.info
Re: asprintf failure
Posted by Bernd Walter <ti...@cicely7.cicely.de>.
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....@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.
Re: asprintf failure
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Mar 28, 2023 at 3:43 PM Fotis Panagiotopoulos
<f....@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.
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