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. 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
