From 46abc3beea42c32cc421907d41ea56fbcf8bc616 Mon Sep 17 00:00:00 2001 From: Chia-Che Tsai Date: Tue, 23 Aug 2016 15:26:27 -0400 Subject: [PATCH] bugfix: snprintf returns wrong # of written characters; and can potentially go overflow --- LibOS/shim/src/utils/printf.c | 16 +++-- Pal/lib/api.h | 4 +- Pal/lib/stdlib/printfmt.c | 121 ++++++++++++++++++-------------- Pal/src/printf.c | 5 +- Pal/src/security/Linux/printf.c | 5 +- 5 files changed, 89 insertions(+), 62 deletions(-) diff --git a/LibOS/shim/src/utils/printf.c b/LibOS/shim/src/utils/printf.c index 3e3daf39..1b22d5fe 100644 --- a/LibOS/shim/src/utils/printf.c +++ b/LibOS/shim/src/utils/printf.c @@ -34,22 +34,26 @@ struct debugbuf { char buf[DEBUGBUF_SIZE]; }; -static inline void +static inline int debug_fputs (void * f, const char * buf, int len) { - DkStreamWrite(debug_handle, 0, len, (void *) buf, NULL); + if (DkStreamWrite(debug_handle, 0, len, (void *) buf, NULL) == len) + return 0; + else + return -1; } -static void +static int debug_fputch (void * f, int ch, void * b) { struct debug_buf * buf = (struct debug_buf *) b; buf->buf[buf->end++] = ch; if (ch == '\n') { - debug_fputs(NULL, buf->buf, buf->end); + if (debug_fputs(NULL, buf->buf, buf->end) == -1) + return -1; buf->end = buf->start; - return; + return 0; } if (buf->end == DEBUGBUF_SIZE - 4) { @@ -61,6 +65,8 @@ debug_fputch (void * f, int ch, void * b) buf->buf[buf->end++] = '.'; buf->buf[buf->end++] = '.'; } + + return 0; } void debug_puts (const char * str) diff --git a/Pal/lib/api.h b/Pal/lib/api.h index 7c7545b6..46abba6a 100644 --- a/Pal/lib/api.h +++ b/Pal/lib/api.h @@ -77,10 +77,10 @@ int memcmp (const void *s1, const void *s2, int len); static_strlen(force_static(str))) /* Libc printf functions */ -void fprintfmt (void (*_fputch)(void *, int, void *), void * f, void * putdat, +void fprintfmt (int (*_fputch)(void *, int, void *), void * f, void * putdat, const char * fmt, ...); -void vfprintfmt (void (*_fputch)(void *, int, void *), void * f, void * putdat, +void vfprintfmt (int (*_fputch)(void *, int, void *), void * f, void * putdat, const char * fmt, va_list *ap); int snprintf (char * buf, int n, const char * fmt, ...); diff --git a/Pal/lib/stdlib/printfmt.c b/Pal/lib/stdlib/printfmt.c index f28fdf4f..2a3e0fc0 100644 --- a/Pal/lib/stdlib/printfmt.c +++ b/Pal/lib/stdlib/printfmt.c @@ -12,26 +12,31 @@ // Print a number (base <= 16) in reverse order, // using specified fputch function and associated pointer putdat. #if !defined(__i386__) -static void -printnum(void (*_fputch)(void *, int, void *), void * f, void * putdat, +static int +printnum(int (*_fputch)(void *, int, void *), void * f, void * putdat, unsigned long long num, unsigned base, int width, int padc) #else -static void -printnum(void (*_fputch)(void *, int, void *), void * f, void * putdat, +static int +printnum(int (*_fputch)(void *, int, void *), void * f, void * putdat, unsigned long num, unsigned base, int width, int padc) #endif { // first recursively print all preceding (more significant) digits if (num >= base) { - printnum(_fputch, f, putdat, num / base, base, width - 1, padc); + if (printnum(_fputch, f, putdat, num / base, base, width - 1, padc) == -1) + return -1; } else { // print any needed pad characters before first digit while (--width > 0) - (*_fputch) (f, padc, putdat); + if ((*_fputch) (f, padc, putdat) == -1) + return -1; } // then print this (the least significant) digit - (*_fputch) (f, "0123456789abcdef"[num % base], putdat); + if ((*_fputch) (f, "0123456789abcdef"[num % base], putdat) == -1) + return -1; + + return 0; } // Get an unsigned int of various possible sizes from a varargs list, @@ -77,12 +82,12 @@ getint(va_list *ap, int lflag) } // Main function to format and print a string. -void fprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, - const char * fmt, ...); +void fprintfmt(int (*_fputch)(void *, int, void *), void * f, void * putdat, + const char * fmt, ...); void -vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, - const char * fmt, va_list *ap) +vfprintfmt(int (*_fputch)(void *, int, void *), void * f, void * putdat, + const char * fmt, va_list *ap) { register const char *p; register int ch; @@ -98,7 +103,8 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, while ((ch = *(unsigned char *) (fmt++)) != '%') { if (ch == '\0') return; - (*_fputch) (f, ch, putdat); + if ((*_fputch) (f, ch, putdat) < 0) + return; } // Process a %-escape sequence @@ -163,7 +169,8 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, // character case 'c': - (*_fputch) (f, va_arg(*ap, int), putdat); + if ((*_fputch) (f, va_arg(*ap, int), putdat) == -1) + return; break; // string @@ -172,14 +179,19 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, p = "(null)"; if (width > 0 && padc != '-') for (width -= strnlen(p, precision); width > 0; width--) - (*_fputch) (f, padc, putdat); + if ((*_fputch) (f, padc, putdat) == -1) + return; for (; (ch = *p++) != '\0' && (precision < 0 || --precision >= 0); width--) - if (altflag && (ch < ' ' || ch > '~')) - (*_fputch) (f, '?', putdat); - else - (*_fputch) (f, ch, putdat); + if (altflag && (ch < ' ' || ch > '~')) { + if ((*_fputch) (f, '?', putdat) == -1) + return; + } else { + if ((*_fputch) (f, ch, putdat) == -1) + return; + } for (; width > 0; width--) - (*_fputch) (f, ' ', putdat); + if ((*_fputch) (f, ' ', putdat) == -1) + return; break; // (signed) decimal @@ -187,12 +199,14 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, num = getint(ap, lflag); #if !defined(__i386__) if ((long long) num < 0) { - (*_fputch) (f, '-', putdat); + if ((*_fputch) (f, '-', putdat) == -1) + return; num = -(long long) num; } #else if ((long) num < 0) { - (*_fputch) (f, '-', putdat); + if ((*_fputch) (f, '-', putdat) == -1) + return; num = -(long) num; } #endif @@ -214,8 +228,10 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, // pointer case 'p': - (*_fputch) (f, '0', putdat); - (*_fputch) (f, 'x', putdat); + if ((*_fputch) (f, '0', putdat) == -1) + return; + if ((*_fputch) (f, 'x', putdat) == -1) + return; #if !defined(__i386__) num = (unsigned long long) (uintptr_t) va_arg(*ap, void *); @@ -231,13 +247,15 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, num = getuint(ap, lflag); base = 16; number: - printnum(_fputch, f, putdat, num, base, width, padc); + if (printnum(_fputch, f, putdat, num, base, width, padc) == -1) + return; break; - // escape character - case '^': - (*_fputch) (f, 0x1b, putdat); - break; + // escape character + case '^': + if ((*_fputch) (f, 0x1b, putdat) == -1) + return; + break; // escaped '%' character case '%': @@ -255,7 +273,7 @@ vfprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, } void -fprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, +fprintfmt(int (*_fputch)(void *, int, void *), void * f, void * putdat, const char * fmt, ...) { va_list ap; @@ -266,46 +284,47 @@ fprintfmt(void (*_fputch)(void *, int, void *), void * f, void * putdat, } struct sprintbuf { - char * buf; - char * ebuf; - int cnt; + int cnt, max; + char * buf; }; -static void +static int sprintputch(void * f, int ch, struct sprintbuf * b) { - b->cnt++; - if (b->buf < b->ebuf) - *b->buf++ = ch; + if (b->cnt >= b->max) + return -1; + + b->buf[b->cnt++] = ch; + return 0; } static int vsprintf(char * buf, int n, const char * fmt, va_list *ap) { - struct sprintbuf b = {buf, buf + n - 1, 0}; + struct sprintbuf b = { 0, n, buf }; - if (buf == NULL || n < 1) { - return -1; - } + if (!buf || n < 1) + return 0; - // print the string to the buffer - vfprintfmt((void *) sprintputch, (void *) 0, &b, fmt, ap); + // print the string to the buffer + vfprintfmt((void *) sprintputch, (void *) 0, &b, fmt, ap); - // null terminate the buffer - *b.buf = '\0'; + // null terminate the buffer + if (b.cnt < n) + b.buf[b.cnt] = '\0'; - return b.cnt; + return b.cnt; } int snprintf(char * buf, int n, const char * fmt, ...) { - va_list ap; - int rc; + va_list ap; + int rc; - va_start(ap, fmt); - rc = vsprintf(buf, n, fmt, &ap); - va_end(ap); + va_start(ap, fmt); + rc = vsprintf(buf, n, fmt, &ap); + va_end(ap); - return rc; + return rc; } diff --git a/Pal/src/printf.c b/Pal/src/printf.c index 6ac8d815..35b94b0d 100644 --- a/Pal/src/printf.c +++ b/Pal/src/printf.c @@ -36,15 +36,16 @@ struct printbuf { char buf[PRINTBUF_SIZE]; }; -static void +static int fputch(void * f, int ch, struct printbuf * b) { b->buf[b->idx++] = ch; - if (b->idx == PRINTBUF_SIZE-1) { + if (b->idx == PRINTBUF_SIZE - 1) { _DkPrintConsole(b->buf, b->idx); b->idx = 0; } b->cnt++; + return 0; } static int diff --git a/Pal/src/security/Linux/printf.c b/Pal/src/security/Linux/printf.c index 5a29b694..f6524748 100644 --- a/Pal/src/security/Linux/printf.c +++ b/Pal/src/security/Linux/printf.c @@ -30,15 +30,16 @@ struct sprintbuf { #define sys_cputs(fd, bf, cnt) INLINE_SYSCALL(write, 3, (fd), (bf), (cnt)) -static void +static int fputch(int fd, int ch, struct printbuf *b) { b->buf[b->idx++] = ch; - if (b->idx == PRINTBUF_SIZE-1) { + if (b->idx == PRINTBUF_SIZE - 1) { sys_cputs(fd, b->buf, b->idx); b->idx = 0; } b->cnt++; + return 0; } static int