printf: Fix hexdump and string 0 precision

This patch addresses a few issues with the current formatter.
The major points are as follows:
1. Cannot specify precision 0 (truncate all) for string or hexdump
2. Forced safe precision for malformed strings
3. No padding when using hexdump
4. Bad error EC_ERROR_INVAL in vsnprintf
5. Documentation errors

For (1), no piece of code explicitly sets the precision to 0 in
order to invoke the default behavior, which is currently no
precision limit.
You can check using the following grep line:
   grep -rI '%[\*0-9]\{0,20\}\.0\{1,20\}[a-zA-Z]'
However, there are many cases where the precision is used to limit
the character output (as it should be).
   grep -rI '%[\*0-9]\{0,20\}\.[\*0-9]\{1,20\}[a-zA-Z]'
There are many more instances that use variable precision without
checking if the precision is zero. One of which is the following:
 crrev.com/4a4e2c71a0f6aaa50e0728922f84a7d54c14380a/test/host_command_fuzz.c#116
 https://clusterfuzz.com/testcase-detail/5699023975088128

Our current implementation will insert ERROR and stop processing,
if a precision of zero is detected when using the hexdump flag.
This results in a badly formatted console line or runtime string,
when the intended behavior would be to simply read no bytes.
In the aforementioned fuzzer case, outputting ERROR triggers
a false positive.
Our printf should handle explicit zero precision similar to
stdlib's printf, which means truncating all the way to zero
positions, if specified.

For (2), our current implementation uses strlen to identify the
length of the input string, regardless of the set precision.
Since this is an embedded platform, we should use strnlen to
impose safe limits, when a precision is specified.

For (3), our implementation should support padding and adjusting
of all formatter types, since that is a primary feature of a
printf formatter.

The remaining commented code highlights odd behavior that should
be fixed at some point, but is not critical.

BUG=chromium:974084
TEST=Checked for any format lines that rely on a set precision of 0
     grep -rI '%[\*0-9]\{0,20\}\.[\*0-9]\{1,20\}[a-zA-Z]'
TEST=make run-printf V=1
BRANCH=none

Change-Id: I897c53cce20a701fcbe8fb9572eb878817525cc3
Signed-off-by: Craig Hesling <hesling@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1659835
Reviewed-by: Randall Spangler <rspangler@chromium.org>

1 files changed, 44 insertions, 14 deletions

diff --git a/common/printf.c b/common/printf.c
index c9bada8bc9..e673c17baa 100644
--- a/common/printf.c
+++ b/common/printf.c
@@ -134,13 +134,14 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 		}
 
 		/* Count precision */
-		precision = 0;
+		precision = -1;
 		if (c == '.') {
 			c = *format++;
 			if (c == '*') {
 				precision = va_arg(args, int);
 				c = *format++;
 			} else {
+				precision = 0;
 				while (c >= '0' && c <= '9') {
 					precision = (10 * precision) + c - '0';
 					c = *format++;
@@ -161,17 +162,40 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 			/* Hex dump output */
 			vstr = va_arg(args, char *);
 
-			if (!precision) {
+			if (precision < 0) {
 				/* Hex dump requires precision */
 				format = error_str;
 				continue;
 			}
 
+			/*
+			 * Divide pad_width instead of multiplying
+			 * precision to avoid overflow error
+			 * in the condition.
+			 * The "/2" and "2*" can be optimized by
+			 * the compiler.
+			 */
+			if ((pad_width/2) >= precision)
+				pad_width -= 2*precision;
+			else
+				pad_width = 0;
+
+			while (pad_width > 0 && !(flags & PF_LEFT)) {
+				if (addchar(context,
+					    flags & PF_PADZERO ? '0' : ' '))
+					return EC_ERROR_OVERFLOW;
+				pad_width--;
+			}
 			for (; precision; precision--, vstr++) {
 				if (addchar(context, hexdigit(*vstr >> 4)) ||
 				    addchar(context, hexdigit(*vstr)))
 					return EC_ERROR_OVERFLOW;
 			}
+			while (pad_width > 0 && (flags & PF_LEFT)) {
+				if (addchar(context, ' '))
+					return EC_ERROR_OVERFLOW;
+				pad_width--;
+			}
 
 			continue;
 		} else {
@@ -260,7 +284,7 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 			 * Fixed-point precision must fit in our buffer.
 			 * Leave space for "0." and the terminating null.
 			 */
-			if (precision > sizeof(intbuf) - 3)
+			if (precision > (int)(sizeof(intbuf) - 3))
 				precision = sizeof(intbuf) - 3;
 
 			/*
@@ -269,7 +293,7 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 			 */
 			for (vlen = 0; vlen < precision; vlen++)
 				*(--vstr) = '0' + divmod(&v, 10);
-			if (precision)
+			if (precision >= 0)
 				*(--vstr) = '.';
 
 			if (!v)
@@ -292,26 +316,32 @@ int vfnprintf(int (*addchar)(void *context, int c), void *context,
 			 * Precision field was interpreted by fixed-point
 			 * logic, so clear it.
 			 */
-			precision = 0;
+			precision = -1;
 		}
 
-		/* Copy string (or stringified integer) */
-		vlen = strlen(vstr);
-
 		/* No padding strings to wider than the precision */
-		if (precision > 0 && pad_width > precision)
+		if (precision >= 0 && pad_width > precision)
 			pad_width = precision;
 
-		/* If precision is zero, print everything */
-		if (!precision)
+		if (precision < 0) {
+			/* If precision is unset, print everything */
+			vlen = strlen(vstr);
 			precision = MAX(vlen, pad_width);
+		} else {
+			/*
+			 * If precision is set, ensure that we do not
+			 * overrun it
+			 */
+			vlen = strnlen(vstr, precision);
+		}
+
 
 		while (vlen < pad_width && !(flags & PF_LEFT)) {
 			if (addchar(context, flags & PF_PADZERO ? '0' : ' '))
 				return EC_ERROR_OVERFLOW;
 			vlen++;
 		}
-		while (*vstr && --precision >= 0)
+		while (--precision >= 0 && *vstr)
 			if (addchar(context, *vstr++))
 				return EC_ERROR_OVERFLOW;
 		while (vlen < pad_width && flags & PF_LEFT) {
@@ -367,8 +397,8 @@ int vsnprintf(char *str, int size, const char *format, va_list args)
 	struct snprintf_context ctx;
 	int rv;
 
-	if (!str || !size)
-		return EC_ERROR_INVAL;
+	if (!str || !format || size <= 0)
+		return -EC_ERROR_INVAL;
 
 	ctx.str = str;
 	ctx.size = size - 1;  /* Reserve space for terminating '\0' */