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, 98 insertions, 31 deletions

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;
 }