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>

3 files changed, 147 insertions, 49 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' */
diff --git a/include/printf.h b/include/printf.h
index 7d9c2ab5e2..ea1445a526 100644
--- a/include/printf.h
+++ b/include/printf.h
@@ -68,7 +68,7 @@
  * @param context	Context pointer to pass to addchar()
  * @param format	Format string (see above for acceptable formats)
  * @param args		Parameters
- * @return EC_SUCCESS, or non-zero if output was truncated.
+ * @return EC_SUCCESS, or EC_ERROR_OVERFLOW if the output was truncated.
  */
 __stdlib_compat int vfnprintf(int (*addchar)(void *context, int c),
 			      void *context, const char *format, va_list args);
@@ -81,12 +81,12 @@ __stdlib_compat int vfnprintf(int (*addchar)(void *context, int c),
  * @param str		Destination string
  * @param size		Size of destination in bytes
  * @param format	Format string
- * @return EC_SUCCESS, or non-zero if output was truncated.
+ * @return EC_SUCCESS, or EC_ERROR_OVERFLOW if the output was truncated.
  */
 __stdlib_compat int snprintf(char *str, int size, const char *format, ...);
 
 /**
- * Print formatted outut to a string.
+ * Print formatted output to a string.
  *
  * Guarantees null-termination if size!=0.
  *
@@ -94,7 +94,8 @@ __stdlib_compat int snprintf(char *str, int size, const char *format, ...);
  * @param size		Size of destination in bytes
  * @param format	Format string
  * @param args		Parameters
- * @return EC_SUCCESS, or non-zero if output was truncated.
+ * @return The string length written to str, or a negative value on error.
+ *         The negative values can be -EC_ERROR_INVAL or -EC_ERROR_OVERFLOW.
  */
 __stdlib_compat int vsnprintf(char *str, int size, const char *format,
 			      va_list args);
diff --git a/test/printf.c b/test/printf.c
index d739da3606..7e857dd55c 100644
--- a/test/printf.c
+++ b/test/printf.c
@@ -3,7 +3,8 @@
  * found in the LICENSE file.
  */
 
-#include <stdarg.h>  /* For va_list */
+#include <stdarg.h>
+#include <stdbool.h>
 #include <stddef.h>
 
 #include "common.h"
@@ -12,33 +13,38 @@
 #include "util.h"
 
 #define INIT_VALUE 0x5E
+#define NO_BYTES_TOUCHED NULL
+
 static const char err_str[] = "ERROR";
 static char output[1024];
 
-int run(const char *expect, size_t size_limit, int expect_ret,
-		const char *format, va_list args)
+int run(int expect_ret, const char *expect,
+	bool output_null, size_t size_limit,
+	const char *format, va_list args)
 {
 	size_t expect_size = expect ? strlen(expect) + 1 : 0;
 	int rv;
 
 	ccprintf("\n");
-	ccprintf("expect='%s'\n", expect);
+	ccprintf("size_limit=%-4d | format='%s'\n", size_limit, format);
+	ccprintf("expect  ='%s'   | expect_status=%d\n",
+		 expect ? expect : "NO_BYTES_TOUCHED", expect_ret);
 
 	TEST_ASSERT(expect_size <= sizeof(output));
 	TEST_ASSERT(expect_size <= size_limit);
 	memset(output, INIT_VALUE, sizeof(output));
 
-	rv = vsnprintf(output, size_limit, format, args);
-	ccprintf("output='%s'\n", output);
+	rv = vsnprintf(output_null ? NULL : output, size_limit,
+		       format, args);
+	ccprintf("received='%.*s'   | ret          =%d\n",
+		 30, output, rv);
 
 	TEST_ASSERT_ARRAY_EQ(output, expect, expect_size);
 	TEST_ASSERT_MEMSET(&output[expect_size], INIT_VALUE,
-						sizeof(output)-expect_size);
+			   sizeof(output) - expect_size);
 
 	if (rv >= 0) {
-		ccprintf("expect_size = %ld\n", expect_size);
-		ccprintf("rv = %d\n", rv);
-		TEST_ASSERT(rv == expect_size-1);
+		TEST_ASSERT(rv == expect_size - 1);
 		TEST_ASSERT(EC_SUCCESS == expect_ret);
 	} else {
 		TEST_ASSERT(rv == -expect_ret);
@@ -53,33 +59,68 @@ int expect_success(const char *expect, const char *format, ...)
 	int rv;
 
 	va_start(args, format);
-	rv = run(expect, sizeof(output), EC_SUCCESS, format, args);
+	rv = run(EC_SUCCESS, expect,
+		 false, sizeof(output),
+		 format, args);
 	va_end(args);
 
 	return rv;
 }
 
-int expect(int expect_ret, size_t size_limit, const char *expect,
-			const char *format, ...)
+int expect(int expect_ret, const char *expect,
+	   bool output_null, size_t size_limit,
+	   const char *format, ...)
 {
 	va_list args;
 	int rv;
 
 	va_start(args, format);
-	rv = run(expect, size_limit, expect_ret, format, args);
+	rv = run(expect_ret, expect,
+		 output_null, size_limit,
+		 format, args);
 	va_end(args);
 
 	return rv;
 }
 
-#define T(n) do { int rv = (n); if (rv != EC_SUCCESS) return rv; } while (0)
+#define T(n)                                                                  \
+	do {                                                                  \
+		int rv = (n);                                                 \
+		if (rv != EC_SUCCESS)                                         \
+			return rv;                                            \
+	} while (0)
 
 test_static int test_vsnprintf_args(void)
 {
 	T(expect_success("",          ""));
 	T(expect_success("a",         "a"));
-	/* T(expect(NULL, 0,              EC_ERROR_INVAL,    "")); fail */
-	T(expect(EC_ERROR_OVERFLOW, 2, "a", "abc"));
+
+	T(expect(/* expect an invalid args error */
+		 EC_ERROR_INVAL, NO_BYTES_TOUCHED,
+		 /* given -1 as output size limit */
+		 false, -1, ""));
+	T(expect(/* expect an invalid args error */
+		 EC_ERROR_INVAL, NO_BYTES_TOUCHED,
+		 /* given 0 as output size limit */
+		 false, 0, ""));
+	T(expect(/* expect SUCCESS */
+		 EC_SUCCESS, "",
+		 /* given 1 as output size limit and a blank format */
+		 false, 1, ""));
+	T(expect(/* expect an overflow error */
+		 EC_ERROR_OVERFLOW, "",
+		 /* given 1 as output size limit with a non-blank format */
+		 false, 1, "a"));
+
+	T(expect(/* expect an invalid args error */
+		 EC_ERROR_INVAL, NO_BYTES_TOUCHED,
+		 /* given NULL as the output buffer */
+		 true, sizeof(output), ""));
+	T(expect(/* expect an invalid args error */
+		 EC_ERROR_INVAL, NO_BYTES_TOUCHED,
+		 /* given a NULL format string */
+		 false, sizeof(output), NULL));
+
 	return EC_SUCCESS;
 }
 
@@ -96,15 +137,28 @@ test_static int test_vsnprintf_int(void)
 	T(expect_success(" +123",     "%+5d",    123));
 	T(expect_success("00123",     "%05d",    123));
 	T(expect_success("00123",     "%005d",   123));
-	/* T(expect_success("+0123",     "%+05d",   123)); actual "0+123" */
-	/* T(expect_success("+0123",     "%+005d",  123)); actual "0+123" */
+	/*
+	 * TODO(crbug.com/974084): This odd behavior should be fixed.
+	 * T(expect_success("+0123",     "%+05d",   123));
+	 * Actual: "0+123"
+	 * T(expect_success("+0123",     "%+005d",  123));
+	 * Actual: "0+123"
+	 */
 
 	T(expect_success("  123",     "%*d",     5, 123));
 	T(expect_success(" +123",     "%+*d",    5, 123));
 	T(expect_success("00123",     "%0*d",    5, 123));
-	/* T(expect_success("00123",     "%00*d",   5, 123)); actual "ERROR" */
+	/*
+	 * TODO(crbug.com/974084): This odd behavior should be fixed.
+	 * T(expect_success("00123",     "%00*d",   5, 123));
+	 * Actual: "ERROR"
+	 */
 	T(expect_success("0+123",     "%+0*d",   5, 123));
-	/* T(expect_success("0+123",     "%+00*d",  5, 123)); actual "ERROR" */
+	/*
+	 * TODO(crbug.com/974084): This odd behavior should be fixed.
+	 * T(expect_success("0+123",     "%+00*d",  5, 123));
+	 * Actual: "ERROR"
+	 */
 
 	T(expect_success("123  ",     "%-5d",    123));
 	T(expect_success("+123 ",     "%-+5d",   123));
@@ -125,7 +179,7 @@ test_static int test_vsnprintf_int(void)
 
 	T(expect_success("123",        "%u",    123));
 	T(expect_success("4294967295", "%u",   -1));
-	T(expect_success("18446744073709551615", "%lu",   (uint64_t)-1));
+	T(expect_success("18446744073709551615", "%lu", (uint64_t)-1));
 
 	T(expect_success("0",         "%x",     0));
 	T(expect_success("0",         "%X",     0));
@@ -163,10 +217,23 @@ test_static int test_vsnprintf_strings(void)
 	T(expect_success("abc",       "%*s",     0, "abc"));
 	T(expect_success("a",         "%.1s",    "abc"));
 	T(expect_success("a",         "%.*s",    1, "abc"));
-	/* T(expect_success("",          "%.0s",    "abc"));    actual "abc" */
-	/* T(expect_success("",          "%.*s",    0, "abc")); actual "abc" */
-	T(expect_success("ab",        "%5.2s",   "abc")); /*    intentional  */
-	/* TODO(hesling): test for overruns on malformed strings */
+	T(expect_success("",          "%.0s",    "abc"));
+	T(expect_success("",          "%.*s",    0, "abc"));
+	/*
+	 * TODO(crbug.com/974084):
+	 * Ignoring the padding parameter is slightly
+	 * odd behavior and could use a review.
+	 */
+	T(expect_success("ab",        "%5.2s",   "abc"));
+	T(expect_success("abc",        "%.4s",   "abc"));
+
+	/*
+	 * Given a malformed string (address 0x1 is a good example),
+	 * if we ask for zero precision, expect no bytes to be read
+	 * from the malformed address and a blank output string.
+	 */
+	T(expect_success("",          "%.0s",    (char *)1));
+
 	return EC_SUCCESS;
 }
 
@@ -176,16 +243,16 @@ test_static int test_vsnprintf_hexdump(void)
 
 	T(expect_success(err_str,     "%h",      bytes));
 	T(expect_success("005e",      "%.*h",    2, bytes));
-	/* T(expect_success("",          "%.*h",    0, bytes));  actual "ERROR" */
-	/* T(expect_success(" 005e",     "%5.*h",   2, bytes));  actual "005e"  */
-	/* T(expect_success("005e ",     "%-5.*h",  2, bytes));  actual "005e"  */
+	T(expect_success("",          "%.*h",    0, bytes));
+	T(expect_success(" 005e",     "%5.*h",   2, bytes));
+	T(expect_success("00",        "%0*.*h",  2, 0, bytes));
 	return EC_SUCCESS;
 }
 
 test_static int test_vsnprintf_combined(void)
 {
-	T(expect_success("abc", "%c%s", 'a', "bc"));
-	T(expect_success("12\tbc", "%d\t%s", 12, "bc"));
+	T(expect_success("abc",       "%c%s",    'a', "bc"));
+	T(expect_success("12\tbc",    "%d\t%s",  12, "bc"));
 	return EC_SUCCESS;
 }