Implementation fixes

The printf(3) man page says "The functions snprintf() and vsnprintf() do
not write more than size bytes (including the terminating null byte
('\0')).  If the output was truncated due to this limit, then the return
value is the number of characters (excluding the terminating null byte)
which would have been written to the final string if enough space had been
available." The last part of this spec (returning the number of
characters which would have been written to the string if enough space
had been available) was not implemented in minimal-printf. This PR
changes that by redirecting all the processed characters through the
"minimal_printf_putchar" helper function. This function will not write
to the buffer if there's no space left, but it will always increment the
number of written characters, in order to match the above description.
minimal_printf_putchar also contains checks for overflows, so these
checks are no longer needed in the rest of the code.

Other changes:

- In some cases, mbed_minimal_formatted_string didn't put the string
  terminator in the output buffer. This PR ensures that this always
  happens.
- Fixed a bug in printed hexadecimal numbers introduced by a previous
  commit.
- Added buffer overflow tests for snprintf.

features/minimal-printf/TESTS/minimal-printf/compliance/main.cpp

@@ -618,6 +618,7 @@ static control_t test_snprintf_x(const size_t call_count)
     return CaseNext;
 }
 
+#if MBED_CONF_MINIMAL_PRINTF_ENABLE_FLOATING_POINT
 static control_t test_printf_f(const size_t call_count)
 {
     int result_baseline;
@@ -672,6 +673,99 @@ static control_t test_snprintf_f(const size_t call_count)
 
     return CaseNext;
 }
+#endif
+
+
+/* Generic buffer overflow test function.
+ * Template parameters:
+    * 'T' is the type being tested
+    * 'buf_size' is the buffer size used in tests
+  * Function parameters:
+    * 'fmt' is the format to use for sprintf
+    * 'data' is the data that will be printed
+*/
+template<typename T, size_t buf_size>
+static control_t test_snprintf_buffer_overflow_generic(const char *fmt, T data)
+{
+    char buffer_baseline[buf_size];
+    char buffer_minimal[buf_size];
+    int result_baseline;
+    int result_minimal;
+
+    /* empty buffer test */
+    result_minimal = mbed_snprintf(buffer_minimal, 0, fmt, data);
+    result_baseline = snprintf(buffer_baseline, 0, fmt, data);
+    TEST_ASSERT_EQUAL_INT(result_baseline, result_minimal);
+
+    /* buffer isn't large enough, output needs to be truncated */
+    result_minimal = mbed_snprintf(buffer_minimal, buf_size - 2, fmt, data);
+    result_baseline = snprintf(buffer_baseline, buf_size - 2, fmt, data);
+    TEST_ASSERT_EQUAL_STRING(buffer_baseline, buffer_minimal);
+    TEST_ASSERT_EQUAL_INT(result_baseline, result_minimal);
+
+    /* buffer is one byte shorter than needed, string terminator must
+       be written and output must be truncated */
+    result_minimal = mbed_snprintf(buffer_minimal, buf_size - 1, fmt, data);
+    result_baseline = snprintf(buffer_baseline, buf_size - 1, fmt, data);
+    TEST_ASSERT_EQUAL_STRING(buffer_baseline, buffer_minimal);
+    TEST_ASSERT_EQUAL_INT(result_baseline, result_minimal);
+
+    /* buffer is just long enough */
+    result_minimal = mbed_snprintf(buffer_minimal, buf_size, fmt, data);
+    result_baseline = snprintf(buffer_baseline, buf_size, fmt, data);
+    TEST_ASSERT_EQUAL_STRING(buffer_baseline, buffer_minimal);
+    TEST_ASSERT_EQUAL_INT(result_baseline, result_minimal);
+
+    return CaseNext;
+}
+
+/* Based on the generic buffer overflow function above, create tests for
+   each relevant data type. In each case, the buffer for printing will only
+   be large enough to fit the printed data. */
+static control_t test_snprintf_buffer_overflow_d(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<int, sizeof("d: -1024")>("d: %d", -1024);
+}
+
+static control_t test_snprintf_buffer_overflow_ld(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<long, sizeof("ld: -1048576")>("ld: %ld", -1048576L);
+}
+
+static control_t test_snprintf_buffer_overflow_lld(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<long long, sizeof("lld: -1099511627776")>("lld: %lld", -1099511627776LL);
+}
+
+static control_t test_snprintf_buffer_overflow_u(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned int, sizeof("u: 1024")>("u: %u", 1024);
+}
+
+static control_t test_snprintf_buffer_overflow_lu(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned long, sizeof("lu: 1048576")>("lu: %lu", 1048576UL);
+}
+
+static control_t test_snprintf_buffer_overflow_llu(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned long long, sizeof("llu: 1099511627776")>("llu: %llu", 1099511627776ULL);
+}
+
+static control_t test_snprintf_buffer_overflow_x(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned int, sizeof("x: 0x400")>("x: 0x%x", 0x400);
+}
+
+static control_t test_snprintf_buffer_overflow_lx(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned long, sizeof("lx: 0x100000")>("lx: 0x%lx", 0x100000UL);
+}
+
+static control_t test_snprintf_buffer_overflow_llx(const size_t call_count)
+{
+    return test_snprintf_buffer_overflow_generic<unsigned long long, sizeof("llx: 0x10000000000")>("llx: 0x%llx", 0x10000000000ULL);
+}
 
 utest::v1::status_t greentea_setup(const size_t number_of_cases)
 {
@@ -690,6 +784,15 @@ Case cases[] = {
     Case("printf %f", test_printf_f),
     Case("snprintf %f", test_snprintf_f),
 #endif
+    Case("snprintf buffer overflow %d", test_snprintf_buffer_overflow_d),
+    Case("snprintf buffer overflow %ld", test_snprintf_buffer_overflow_ld),
+    Case("snprintf buffer overflow %lld", test_snprintf_buffer_overflow_lld),
+    Case("snprintf buffer overflow %u", test_snprintf_buffer_overflow_u),
+    Case("snprintf buffer overflow %lu", test_snprintf_buffer_overflow_lu),
+    Case("snprintf buffer overflow %llu", test_snprintf_buffer_overflow_llu),
+    Case("snprintf buffer overflow %x", test_snprintf_buffer_overflow_x),
+    Case("snprintf buffer overflow %lx", test_snprintf_buffer_overflow_lx),
+    Case("snprintf buffer overflow %llx", test_snprintf_buffer_overflow_llx),
 };
 
 Specification specification(greentea_setup, cases, greentea_test_teardown_handler);

features/minimal-printf/mbed_printf_implementation.c

@@ -133,6 +133,37 @@ static void mbed_minimal_formatted_string_void_pointer(char* buffer, size_t leng
 static void mbed_minimal_formatted_string_character(char* buffer, size_t length, int* result, char character);
 static void mbed_minimal_formatted_string_string(char* buffer, size_t length, int* result, const char* string);
 
+/**
+ * @brief      Print a single character, checking for buffer and size overflows.
+ *
+ * @param      buffer  The buffer to store output (NULL for stdout).
+ * @param[in]  length  The length of the buffer.
+ * @param      result  The current output location.
+ * @param[in]  data    The char to be printed.
+ */
+static void mbed_minimal_putchar(char *buffer, size_t length, int* result, char data)
+{
+    /* only continue if 'result' doesn't overflow */
+    if ((*result >= 0) && (*result <= INT_MAX - 1))
+    {
+        /* write data only if there's enough space */
+        if ((size_t)*result < length)
+        {
+            if (buffer)
+            {
+                buffer[*result] = data;
+            }
+            else
+            {
+                MBED_PRINT_CHARACTER(data);
+            }
+        }
+        /* increment 'result' even if data was not written. This ensures that
+           'mbed_minimal_formatted_string' returns the correct value. */
+        *result += 1;
+    }
+}
+
 /**
  * @brief      Print signed integer.
  *
@@ -143,38 +174,24 @@ static void mbed_minimal_formatted_string_string(char* buffer, size_t length, in
  */
 static void mbed_minimal_formatted_string_signed(char* buffer, size_t length, int* result, MBED_SIGNED_STORAGE value)
 {
-    /* only continue if buffer can fit at least 1 character and if
+    MBED_UNSIGNED_STORAGE new_value = 0;
-       'result' doesn't overflow */
+
-    if ((*result >= 0) && (*result <= INT_MAX - 1) && ((size_t)*result + 1 <= length))
+    /* if value is negative print sign and treat as positive number */
+    if (value < 0)
     {
-        MBED_UNSIGNED_STORAGE new_value = 0;
+        /* write sign */
+        mbed_minimal_putchar(buffer, length, result, '-');
 
-        /* if value is negative print sign and treat as positive number */
+        /* get absolute value using two's complement */
-        if (value < 0)
+        new_value = ~((MBED_UNSIGNED_STORAGE) value) + 1;
-        {
-            /* write sign */
-            if (buffer)
-            {
-                buffer[*result] = '-';
-            }
-            else
-            {
-                MBED_PRINT_CHARACTER('-');
-            }
-
-            *result += 1;
-
-            /* get absolute value using two's complement */
-            new_value = ~((MBED_UNSIGNED_STORAGE) value) + 1;
-        }
-        else
-        {
-            new_value = value;
-        }
-
-        /* use unsigned long int function */
-        mbed_minimal_formatted_string_unsigned(buffer, length, result, new_value);
     }
+    else
+    {
+        new_value = value;
+    }
+
+    /* use unsigned long int function */
+    mbed_minimal_formatted_string_unsigned(buffer, length, result, new_value);
 }
 
 /**
@@ -187,55 +204,32 @@ static void mbed_minimal_formatted_string_signed(char* buffer, size_t length, in
  */
 static void mbed_minimal_formatted_string_unsigned(char* buffer, size_t length, int* result, MBED_UNSIGNED_STORAGE value)
 {
-    /* only continue if buffer can fit at least 1 character and if
+    /* treat 0 as a corner case */
-       'result' doesn't overflow */
+    if (value == 0)
-    if ((*result >= 0) && (*result <= INT_MAX - 1) && ((size_t)*result + 1 <= length))
     {
-        /* treat 0 as a corner case */
+        mbed_minimal_putchar(buffer, length, result, '0');
-        if (value == 0)
+    }
-        {
+    else
-            if (buffer)
+    {
-            {
+        /* allocate 3 digits per byte */
-                buffer[*result] = '0';
+        char scratch[sizeof(MBED_UNSIGNED_STORAGE) * 3] = { 0 };
-            }
-            else
-            {
-                MBED_PRINT_CHARACTER('0');
-            }
 
-            *result += 1;
+        size_t index = 0;
+
+        /* write numbers in reverse order to scratch pad */
+        for ( ; value > 0; index++)
+        {
+            /* use '0' as base and add digit */
+            scratch[index] = '0' + (value % 10);
+
+            /* shift value one decimal position */
+            value = value / 10;
         }
-        else
+
+        /* write scratch pad to buffer or output */
+        for ( ; index > 0; index--)
         {
-            /* allocate 3 digits per byte */
+            mbed_minimal_putchar(buffer, length, result, scratch[index - 1]);
-            char scratch[sizeof(MBED_UNSIGNED_STORAGE) * 3] = { 0 };
-
-            size_t index = 0;
-
-            /* write numbers in reverse order to scratch pad */
-            for ( ; value > 0; index++)
-            {
-                /* use '0' as base and add digit */
-                scratch[index] = '0' + (value % 10);
-
-                /* shift value one decimal position */
-                value = value / 10;
-            }
-
-            /* write scratch pad to buffer or output */
-            for ( ; (*result <= INT_MAX- 1) && ((size_t)*result < length) && (index > 0); index--)
-            {
-                if (buffer)
-                {
-                    buffer[*result] = scratch[index - 1];
-                }
-                else
-                {
-                    MBED_PRINT_CHARACTER(scratch[index - 1]);
-                }
-
-                *result += 1;
-            }
         }
     }
 }
@@ -252,9 +246,7 @@ static void mbed_minimal_formatted_string_hexadecimal(char* buffer, size_t lengt
 {
     bool print_leading_zero = false;
 
-    /* only continue each loop if buffer can fit at least 2 characters
+    for (int index = 7; index >= 0; index--)
-       and if 'result' doesn't overflow */
-    for (int index = 7; (*result >= 0) && (*result <= INT_MAX - 2) && ((size_t)*result + 2 <= length) && (index >= 0); index--)
     {
         /* get most significant byte */
         uint8_t output = value >> (8 * index);
@@ -262,35 +254,19 @@ static void mbed_minimal_formatted_string_hexadecimal(char* buffer, size_t lengt
         /* only print leading zeros when set */
         if (print_leading_zero || (output != 0) || (index == 0))
         {
-            /* print zeroes after the first non-zero byte */
-            print_leading_zero = true;
-
             unsigned int nibble_one = (output >> 4);
             unsigned int nibble_two = (output & 0x0F);
 
             const char int2hex[16] = { '0', '1', '2', '3', '4', '5', '6', '7',
                                        '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
 
-            /* write to buffer or stdout */
+            if (print_leading_zero || nibble_one != 0) {
-            if (buffer)
+                mbed_minimal_putchar(buffer, length, result, int2hex[nibble_one]);
-            {
-                if (nibble_one != 0) {
-                    buffer[*result] = int2hex[nibble_one];
-                    *result += 1;
-                }
-                buffer[*result] = int2hex[nibble_two];
-                *result += 1;
-            }
-            else
-            {
-                if (nibble_one != 0) {
-                    MBED_PRINT_CHARACTER(int2hex[nibble_one]);
-                    *result += 1;
-                }
-                MBED_PRINT_CHARACTER(int2hex[nibble_two]);
-                *result += 1;
             }
+            mbed_minimal_putchar(buffer, length, result, int2hex[nibble_two]);
 
+            /* print zeroes after the first non-zero byte */
+            print_leading_zero = true;
         }
     }
 }
@@ -305,28 +281,12 @@ static void mbed_minimal_formatted_string_hexadecimal(char* buffer, size_t lengt
  */
 static void mbed_minimal_formatted_string_void_pointer(char* buffer, size_t length, int* result, const void* value)
 {
-    /* only continue if buffer can fit '0x' and twice the size of a void*
+    /* write leading 0x */
-       and if 'result' doesn't overflow */
+    mbed_minimal_putchar(buffer, length, result, '0');
-    size_t needed = 2 + 2 * sizeof(void*);
+    mbed_minimal_putchar(buffer, length, result, 'x');
-    if ((*result >= 0) && ((size_t)*result <= INT_MAX - needed) && ((size_t)*result + needed <= length))
-    {
-        /* write leading 0x */
-        if (buffer)
-        {
-            buffer[*result] = '0';
-            buffer[*result + 1] = 'x';
-        }
-        else
-        {
-            MBED_PRINT_CHARACTER('0');
-            MBED_PRINT_CHARACTER('x');
-        }
 
-        *result += 2;
+    /* write rest as a regular hexadecimal number */
-
+    mbed_minimal_formatted_string_hexadecimal(buffer, length, result, (ptrdiff_t) value);
-        /* write rest as a regular hexadecimal number */
-        mbed_minimal_formatted_string_hexadecimal(buffer, length, result, (ptrdiff_t) value);
-    }
 }
 
 #if MBED_CONF_MINIMAL_PRINTF_ENABLE_FLOATING_POINT
@@ -340,53 +300,48 @@ static void mbed_minimal_formatted_string_void_pointer(char* buffer, size_t leng
  */
 static void mbed_minimal_formatted_string_double(char* buffer, size_t length, int* result, double value)
 {
-    /* only continue if buffer can fit at least 1 character and if
+    /* get integer part */
-       'result' doesn't overflow */
+    MBED_SIGNED_STORAGE integer = value;
-    if ((*result >= 0) && (*result <= INT_MAX - 1) && ((size_t)*result + 1 <= length))
+
+    /* write integer part */
+    mbed_minimal_formatted_string_signed(buffer, length, result, integer);
+
+    /* write decimal point */
+    mbed_minimal_formatted_string_character(buffer, length, result, '.');
+
+    /* get decimal part */
+    double precision = 1.0;
+
+    for (size_t index = 0; index < MBED_CONF_MINIMAL_PRINTF_SET_FLOATING_POINT_MAX_DECIMALS; index++)
     {
-        /* get integer part */
+        precision *= 10;
-        MBED_SIGNED_STORAGE integer = value;
-
-        /* write integer part */
-        mbed_minimal_formatted_string_signed(buffer, length, result, integer);
-
-        /* write decimal point */
-        mbed_minimal_formatted_string_character(buffer, length, result, '.');
-
-        /* get decimal part */
-        double precision = 1.0;
-
-        for (size_t index = 0; index < MBED_CONF_MINIMAL_PRINTF_SET_FLOATING_POINT_MAX_DECIMALS; index++)
-        {
-            precision *= 10;
-        }
-
-        value = (value - integer) * precision;
-
-        /* convert to unsigned integer */
-        MBED_UNSIGNED_STORAGE decimal = 0;
-
-        if (value < 0)
-        {
-            MBED_SIGNED_STORAGE temp = value;
-            decimal = ~((MBED_UNSIGNED_STORAGE) temp) + 1;
-        }
-        else
-        {
-            decimal = value;
-        }
-
-        /* round up or down */
-        value -= decimal;
-
-        if (!((value > -0.5) && (value < 0.5)))
-        {
-            decimal++;
-        }
-
-        /* write decimal part */
-        mbed_minimal_formatted_string_unsigned(buffer, length, result, decimal);
     }
+
+    value = (value - integer) * precision;
+
+    /* convert to unsigned integer */
+    MBED_UNSIGNED_STORAGE decimal = 0;
+
+    if (value < 0)
+    {
+        MBED_SIGNED_STORAGE temp = value;
+        decimal = ~((MBED_UNSIGNED_STORAGE) temp) + 1;
+    }
+    else
+    {
+        decimal = value;
+    }
+
+    /* round up or down */
+    value -= decimal;
+
+    if (!((value > -0.5) && (value < 0.5)))
+    {
+        decimal++;
+    }
+
+    /* write decimal part */
+    mbed_minimal_formatted_string_unsigned(buffer, length, result, decimal);
 }
 #endif
 
@@ -400,34 +355,24 @@ static void mbed_minimal_formatted_string_double(char* buffer, size_t length, in
  */
 static void mbed_minimal_formatted_string_character(char* buffer, size_t length, int* result, char character)
 {
-    /* only continue if the buffer can fit 1 character and if
+    /* write character */
-       'result' doesn't overflow */
+    if (buffer)
-    if ((*result >= 0) && (*result <= INT_MAX - 1) && ((size_t)*result + 1 <= length))
     {
-        /* write character */
+        mbed_minimal_putchar(buffer, length, result, character);
-        if (buffer)
+    }
-        {
+    else
-            buffer[*result] = character;
+    {
-        }
+        /* convert \n to \r\n if enabled in platform configuration */
-        else
-        {
-            /* convert \n to \r\n if enabled in platform configuration */
 #if MBED_CONF_PLATFORM_STDIO_CONVERT_NEWLINES
-            if (character == '\n' && mbed_stdio_out_prev != '\r')
+        if (character == '\n' && mbed_stdio_out_prev != '\r')
-            {
+        {
-                MBED_PRINT_CHARACTER('\r');
+            mbed_minimal_putchar(buffer, length, result, '\r');
-                *result += 1;
-            }
-
-            /* cache character */
-            mbed_stdio_out_prev = character;
-#endif
-
-            /* write character to stdout */
-            MBED_PRINT_CHARACTER(character);
         }
 
-        *result += 1;
+        /* cache character */
+        mbed_stdio_out_prev = character;
+#endif
+        mbed_minimal_putchar(buffer, length, result, character);
     }
 }
 
@@ -441,44 +386,10 @@ static void mbed_minimal_formatted_string_character(char* buffer, size_t length,
  */
 static void mbed_minimal_formatted_string_string(char* buffer, size_t length, int* result, const char* string)
 {
-    /* only continue if the buffer can fit at least 1 character and if
+    while (*string != '\0')
-       'result' doesn't overflow */
-    if ((*result >= 0) && (*result <= INT_MAX - 1) && ((size_t)*result + 1 <= length))
     {
-        /* count characters in string */
+        mbed_minimal_putchar(buffer, length, result, *string);
-        size_t remaining = length - *result;
+        string ++;
-        size_t string_length = 0;
-
-        /* only count characters that will fit into buffer */
-        while ((string[string_length] != '\0') && (string_length < remaining))
-        {
-            string_length++;
-        }
-
-        /* copy string to buffer */
-        if (buffer)
-        {
-            /* ensure that the value of "result" doesn't overflow */
-            if (string_length + *result > INT_MAX)
-            {
-                string_length = (size_t)INT_MAX - *result;
-            }
-            for (size_t index = 0; index < string_length; index++)
-            {
-                buffer[*result + index] = string[index];
-            }
-        }
-        /* print string */
-        else
-        {
-            for (size_t index = 0; index < string_length; index++)
-            {
-                MBED_PRINT_CHARACTER(string[index]);
-            }
-        }
-
-        /* add length to counter */
-        *result += string_length;
     }
 }
 
@@ -498,12 +409,23 @@ int mbed_minimal_formatted_string(char* buffer, size_t length, const char* forma
     MBED_INITIALIZE_PRINT();
 
     int result = 0;
+    bool empty_buffer = false;
 
     /* ensure that function wasn't called with an empty buffer, or with or with
        a buffer size that is larger than the maximum 'int' value, or with
        a NULL format specifier */
-    if (format && length > 0 && length <= INT_MAX)
+    if (format && length >= 0 && length <= INT_MAX)
     {
+        /* Make sure that there's always space for the NULL terminator */
+        if (length > 0)
+        {
+            length --;
+        }
+        else
+        {
+            /* the buffer is empty, there's no place to write the terminator */
+            empty_buffer = true;
+        }
         /* parse string */
         for (size_t index = 0; format[index] != '\0'; index++)
         {
@@ -765,10 +687,18 @@ int mbed_minimal_formatted_string(char* buffer, size_t length, const char* forma
             }
         }
 
-        /* if writing to buffer, NULL terminate string in reserved space*/
+        if (buffer && !empty_buffer)
-        if (buffer && ((size_t)result < length))
         {
-            buffer[result] = '\0';
+            /* NULL-terminate the buffer no matter what. We use '<=' to compare instead of '<'
+               because we know that we initially reserved space for '\0' by decrementing length */
+            if ((size_t)result <= length)
+            {
+                buffer[result] = '\0';
+            }
+            else
+            {
+                buffer[length] = '\0';
+            }
         }
     }