Hello posh folks,
I took another look at this section of code, and found another out-of-bounds...
but this time an underflow:
tree.c:425: *--p = '-';
The above writes a '-' 1 byte before the beginning of the malloc'd buffer
pointed to by p. So, it looks like this implementation potentially exposes a
buffer underflow and overflow. I have a patch to offer here that fixes all
(hopefully email wont mangle it this time:
```
--- src/posh-0.14.2/tree.c 2013-05-13 02:16:42.000000000 +0800
+++ tree.c 2025-12-02 17:31:53.332119894 +0800
@@ -2,6 +2,7 @@
* command tree climbing
*/
+#include <alloca.h>
#include "sh.h"
#define INDENT 4
@@ -393,12 +394,12 @@
static void vfptreef(struct shf *shf, int indent, const char *fmt, va_list va)
{
int c;
+ char *buf; /* for integer -> string conversions */
+ const size_t sz = 24; /* safe for integer conversions */
while ((c = *fmt++))
if (c == '%') {
- long n;
- char *p, *q;
- int neg;
+ char *p;
switch ((c = *fmt++)) {
case 'c':
@@ -413,20 +414,21 @@
p = va_arg(va, char *);
tputS(p, shf);
break;
- case 'd': case 'u': /* decimal */
- n = (c == 'd') ? va_arg(va, int)
- : va_arg(va, unsigned int);
- neg = c=='d' && n<0;
- p = q = malloc(20);
- snprintf(p, 19, "%ld", (neg) ? -n : n);
- p[20] = '\0';
-
- if (neg)
- *--p = '-';
+ case 'd':
+ if (buf == NULL)
+ buf = alloca(sz);
+ p = buf;
+ snprintf(p, sz, "%d", va_arg(va, int));
+ while (*p)
+ tputc(*p++, shf);
+ break;
+ case 'u':
+ if (buf == NULL)
+ buf = alloca(sz);
+ p = buf;
+ snprintf(p, sz, "%u", va_arg(va, unsigned int));
while (*p)
tputc(*p++, shf);
-
- free(q);
break;
case 'T': /* format tree */
ptree(va_arg(va, struct op *), indent, shf);
```
Notes about the patch:
The previous implementation ostensibly didn't trust that snprintf() would
correctly translate negative numbers & attempted to support them with some
extra logic and manipulation via pointers. Likely, the original code posh
inherited was built on sprintf() -- that had this issue in very early versions
of the C library. However... we are now using snprintf(). By the time that
snprintf() was included in the C standard library, that particular behavior was
fixed. We should be able to safely simplify the code such that we just let
snprintf() handle negative numbers for us.
Also, because we are using snprintf() in the code, another C lib feature also
matured at the same time -- that being the behavior of alloca() -- that enables
stack allocation rather than heap allocation. In the new implementation, we
continue with the spirit of the code in terms of being very resource stingy and
only alloca() when we need to; and, additionally, we keep and re-use the
allocation until we exit the function to keep it fast -- and it should also be
faster than the heap based method where we malloc'd and free'd every loop
iteration.
Finally, I broke out case 'u' from case 'd', as the very minor duplication
makes the code brain-dead simple to reason about.
Review notes:
- I pulled the 'l' from the format string in snprintf()... The reason I did so
is that the types are defined in the va_arg(), and I don't think we need to use
the larger type.
- A second note for review is that we stick with the original approach of not
checking the result of snprintf() > 0 -- This is in the spirit of the original
code, and I don't know that it matters.
- A final review note is that though we do not check the result of alloca() I
believe that is not as important as checking the result of malloc()... An
alloca() with such a small allocation should never overflow the stack -- and
the host would have a much bigger systemic problem if it did.
Michael Back <[email protected]>
