Skip to content
Permalink
Browse files

prf.c: remove buffer limitation on field width and string copy

The z_prf() function currently allocates a 200-byte buffer on the
stack to copy strings into, and then perform left/right alignment
and padding. Not only this is a pretty large chunk of stack usage,
but this imposes limitations on field width and string length. Also
the string is copied not only once but _thrice_ making this code
less than optimal.

Let's rework the code to get rid of both the field width limit and
string length limit, as well as the two extra memory copy instances.

While at it, let's fixes printf("%08s", "abcd") which used to
produce "0000abcd".

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
  • Loading branch information...
Nicolas Pitre authored and nashif committed Jun 18, 2019
1 parent 4ad2a8f commit 33312cfd98f80fa9923e6009d4caa1f977059189
Showing with 47 additions and 80 deletions.
  1. +44 −64 lib/libc/minimal/source/stdout/prf.c
  2. +3 −16 tests/lib/sprintf/src/main.c
@@ -12,6 +12,7 @@
#include <stdarg.h>
#include <string.h>
#include <ctype.h>
#include <limits.h>
#include <sys/types.h>
#include <sys/util.h>

@@ -455,7 +456,6 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
char *cptr;
bool falt, fminus, fplus, fspace;
int i;
bool need_justifying;
char pad;
int precision;
int prefix;
@@ -473,7 +473,6 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
} else {
fminus = fplus = fspace = falt = false;
pad = ' '; /* Default pad character */
precision = -1; /* No precision specified */

while (strchr("-+ #0", (c = *format++)) != NULL) {
switch (c) {
@@ -517,16 +516,7 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
c = *format++;
}

/*
* If <width> is INT_MIN, then its absolute value can
* not be expressed as a positive number using 32-bit
* two's complement. To cover that case, cast it to
* an unsigned before comparing it against MAXFLD.
*/
if ((unsigned) width > MAXFLD) {
width = MAXFLD;
}

precision = -1;
if (c == '.') {
c = *format++;
if (c == '*') {
@@ -565,14 +555,13 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
}
}

need_justifying = false;
cptr = buf;
prefix = 0;
switch (c) {
case 'c':
buf[0] = va_arg(vargs, int);
buf[1] = '\0';
need_justifying = true;
c = 1;
pad = ' ';
break;

case 'd':
@@ -599,8 +588,7 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
if (fplus || fspace || val < 0) {
prefix = 1;
}
need_justifying = true;
if (precision != -1) {
if (precision >= 0) {
pad = ' ';
}
break;
@@ -637,7 +625,6 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
if (fplus || fspace || (buf[0] == '-')) {
prefix = 1;
}
need_justifying = true;
break;
}

@@ -664,35 +651,29 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
*va_arg(vargs, int *) = count;
break;
}
break;
continue;

case 'p':
val = (uintptr_t) va_arg(vargs, void *);
c = _to_hex(buf, val, true, 2*sizeof(void *), 'x');
need_justifying = true;
if (precision != -1) {
if (precision >= 0) {
pad = ' ';
}
break;

case 's':
{
char *cptr_temp = va_arg(vargs, char *);
cptr = va_arg(vargs, char *);
/* Get the string length */
for (c = 0; c < MAXFLD; c++) {
if (cptr_temp[c] == '\0') {
if (precision < 0) {
precision = INT_MAX;
}
for (c = 0; c < precision; c++) {
if (cptr[c] == '\0') {
break;
}
}
if ((precision >= 0) && (precision < c)) {
c = precision;
}
if (c > 0) {
memcpy(buf, cptr_temp, (size_t) c);
need_justifying = true;
}
pad = ' ';
break;
}

case 'o':
case 'u':
@@ -726,57 +707,56 @@ int z_prf(int (*func)(), void *dest, const char *format, va_list vargs)
prefix = 2;
}
}
need_justifying = true;
if (precision != -1) {
if (precision >= 0) {
pad = ' ';
}
break;

case '%':
PUTC('%');
count++;
break;
continue;

default:
PUTC('%');
PUTC(c);
count += 2;
break;
continue;

case 0:
return count;
}

if (c >= MAXFLD + 1) {
return EOF;
/* padding for right justification */
if (!fminus && c < width) {
if (pad == ' ') {
prefix = 0;
}
width -= prefix;
c -= prefix;
count += prefix;
while (prefix-- > 0) {
PUTC(*cptr++);
}
width -= c;
count += width;
while (width-- > 0) {
PUTC(pad);
}
}

if (need_justifying) {
if (c < width) {
if (fminus) {
/* Left justify? */
for (i = c; i < width; i++) {
buf[i] = ' ';
}
} else {
/* Right justify */
(void) memmove((buf + (width - c)), buf, (size_t) (c
+ 1));
if (pad == ' ') {
prefix = 0;
}

c = width - c + prefix;
for (i = prefix; i < c; i++) {
buf[i] = pad;
}
}
c = width;
}
/* data out */
width -= c;
count += c;
while (c-- > 0) {
PUTC(*cptr++);
}

count += c;
for (cptr = buf; c > 0; c--, cptr++) {
PUTC(*cptr);
/* padding for left justification */
if (width > 0) {
count += width;
while (width-- > 0) {
PUTC(' ');
}
}
}
@@ -45,8 +45,6 @@
"66666666666666666666666666666666" \
"666666666666666666666666666666666"

#define PRINTF_MAX_STRING_LENGTH 200

union raw_double_u {
double d;
struct {
@@ -580,7 +578,6 @@ void test_sprintf_integer(void)

void test_sprintf_string(void)
{
int len;
char buffer[400];

sprintf(buffer, "%%");
@@ -596,19 +593,9 @@ void test_sprintf_string(void)
"sprintf(%%s). "
"Expected 'short string', got '%s'\n", buffer);

len = sprintf(buffer, "%s", REALLY_LONG_STRING);
#if !defined CONFIG_NEWLIB_LIBC && !defined CONFIG_ARCH_POSIX
zassert_true((len == PRINTF_MAX_STRING_LENGTH),
"Internals changed. "
"Max string length no longer %d got %d\n",
PRINTF_MAX_STRING_LENGTH, len);
#endif
zassert_true((strncmp(buffer, REALLY_LONG_STRING,
PRINTF_MAX_STRING_LENGTH) == 0),
"First %d characters of REALLY_LONG_STRING "
"not printed!\n",
PRINTF_MAX_STRING_LENGTH);

sprintf(buffer, "%s", REALLY_LONG_STRING);
zassert_true((strcmp(buffer, REALLY_LONG_STRING) == 0),
"sprintf(%%s) of REALLY_LONG_STRING doesn't match!\n");
}

/**

0 comments on commit 33312cf

Please sign in to comment.
You can’t perform that action at this time.