From: Oliver Kurth <okurth@vmware.com>
Date: Fri, 15 Sep 2017 18:23:56 +0000 (-0700)
Subject: Fix lib/string unit tests
X-Git-Tag: stable-10.2.0~53
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e30c97abc0fb8f0f6034d56d93b63b629365e2cb;p=thirdparty%2Fopen-vm-tools.git

Fix lib/string unit tests

Updates to lib/string.str.c and its unit tests.

* On Linux, Str_Vsnwprintf uses vswprintf from glibc[1].
However, glibc's vswprintf implementation appears to be
broken by not NUL-terminating the destination if truncation
occurs.  Make Str_Vsnwprintf explicitly NUL-terminate along
this path.

* The unit tests expected that Str_Vsnwprintf use %s for
wchar_t* arguments (and %S for char* ones).  That is a
Microsoft-ism (necessary for TCHAR-based printf functions
to use the same format strings with or without _UNICODE).
Our bsd_vsnwprintf function instead uses %s for char* (and
%S for wchar_t*).  Existing callers to Str_...wprintf use
use the BSD implementation and expect the BSD behavior
(which is consistent with standard C in this regard), so
go with the existing behavior and adjust the tests.

* Some of the tests passed int arguments for floating-point
format specifiers.  This is undefined behavior.

* Some of the tests had more format specifiers than
arguments.  More undefined behavior.

* Finally, the existing Str_...printf tests are
fundamentally flawed: they compare output against the
system's printf functions.  However, the whole point of
having our own is to have consistent behavior across
platforms and to work around bugs; the output therefore
cannot be expected to consistently match.  Instead rewrite
the tests to compare Str_...printf output against fixed
strings.[2]  Additionally, enable these tests only if
HAS_BSD_PRINTF/HAS_BSD_WPRINTF are enabled; I see no point
in testing the system versions (which won't have consistent
output), and IMO the BSD implementations are the only ones
we should be supporting.

[1] I do not understand why HAS_BSD_WPRINTF is enabled on
Linux only for < gcc 2.96 (and not at all for macOS).
This probably should be fixed, but not as part of this
change. (I also don't know why HAS_BSD_WPRINTF is
distinct from HAS_BSD_PRINTF.)

[2] bsd_vsnwprintf is not consistent with bsd_vsnprintf for
all format specifiers.  I plan to deal with this later,
but for now, make the expected strings match the actual
output so we have a starting point where everything
passes.
---

diff --git a/open-vm-tools/lib/string/str.c b/open-vm-tools/lib/string/str.c
index 79e77cab9..c1a61a1f7 100644
--- a/open-vm-tools/lib/string/str.c
+++ b/open-vm-tools/lib/string/str.c
@@ -83,15 +83,20 @@ extern int vswprintf(wchar_t *wcs, size_t maxlen, const wchar_t *format, va_list
  *      Compatibility wrapper b/w different libc versions
  *
  * Results:
+ *      int - number of bytes stored in 'str' (not including NUL terminator),
+ *      -1 on overflow (insufficient space for NUL terminator is considered
+ *      overflow).
  *
- *      int - number of bytes stored in 'str' (not including NUL
- *      terminate character), -1 on overflow (insufficient space for
- *      NUL terminate is considered overflow)
+ *      Guaranteed to NUL-terminate if 'size' > 0.
  *
  *      NB: on overflow the buffer WILL be NUL terminated at the last
  *      UTF-8 code point boundary within the buffer's bounds.
  *
- * WARNING: See warning at the top of this file.
+ * WARNING:
+ *      Behavior of this function is guaranteed only if HAS_BSD_PRINTF is
+ *      enabled.
+ *
+ *      See the warning at the top of this file for proper va_list usage.
  *
  * Side effects:
  *      None
@@ -113,19 +118,17 @@ Str_Vsnprintf(char *str,          // OUT
 #if defined HAS_BSD_PRINTF && !defined __ANDROID__
    retval = bsd_vsnprintf(&str, size, format, ap);
 #else
-   retval = vsnprintf(str, size, format, ap);
-#endif
-
    /*
-    * Linux glibc 2.0.x returns -1 and NUL terminates (which we shouldn't
-    * be linking against), but glibc 2.1.x follows c99 and returns
-    * characters that would have been written.
+    * Linux glibc 2.0.x (which we shouldn't be linking against) returns -1, but
+    * glibc 2.1.x follows c99 and returns the number characters (excluding NUL)
+    * that would have been written if given a sufficiently large buffer.
     *
-    * In the case of Win32 and !HAS_BSD_PRINTF, we are using
-    * _vsnprintf(), which returns -1 on overflow, returns size
-    * when result fits exactly, and does not NUL terminate in
-    * those cases.
+    * In the case of Win32, this path uses _vsnprintf(), which returns -1 on
+    * overflow, returns size when result fits exactly, and does not NUL
+    * terminate in those cases.
     */
+   retval = vsnprintf(str, size, format, ap);
+#endif
 
    if ((retval < 0) || (retval >= size)) {
       if (size > 0) {
@@ -236,12 +239,7 @@ Str_Sprintf(char *buf,       // OUT
  *      Compatibility wrapper b/w different libc versions
  *
  * Results:
- *
- *      int - number of bytes stored in 'str' (not including NUL
- *      terminate character), -1 on overflow (insufficient space for
- *      NUL terminate is considered overflow)
- *
- *      NB: on overflow the buffer WILL be NUL terminated
+ *      See Str_Vsnprintf.
  *
  * Side effects:
  *      None
@@ -435,6 +433,8 @@ Str_Strcat(char *buf,       // IN/OUT
  *    Specifically, this function will Panic if a buffer overrun would
  *    have occurred.
  *
+ *    Guaranteed to NUL-terminate if bufSize > 0.
+ *
  * Results:
  *    Same as strncat.
  *
@@ -461,7 +461,6 @@ Str_Strncat(char *buf,       // IN/OUT
     * enough... so repeat the check using strlen(src).
     *
     * The "fit" means less than, as strncat always adds a terminating NUL.
-    *
     */
 
    bufLen = strlen(buf);
@@ -693,11 +692,15 @@ Str_SafeVasprintf(size_t *length,       // OUT/OPT
  *
  * Str_Swprintf --
  *
- *      wsprintf wrapper that fails on overflow
+ *      swprintf wrapper that fails on overflow
  *
  * Results:
  *      Returns the number of wchar_ts stored in 'buf'.
  *
+ * WARNING:
+ *      Behavior of this function is guaranteed only if HAS_BSD_WPRINTF is
+ *      enabled.
+ *
  * Side effects:
  *      None.
  *
@@ -738,7 +741,11 @@ Str_Swprintf(wchar_t *buf,       // OUT
  *
  *      NB: on overflow the buffer WILL be NUL terminated
  *
- * WARNING: See warning at the top of this file.
+ * WARNING:
+ *      Behavior of this function is guaranteed only if HAS_BSD_WPRINTF is
+ *      enabled.
+ *
+ *      See the warning at the top of this file for proper va_list usage.
  *
  * Side effects:
  *      None
@@ -754,30 +761,32 @@ Str_Vsnwprintf(wchar_t *str,          // OUT
 {
    int retval;
 
-#if defined(HAS_BSD_WPRINTF) && HAS_BSD_WPRINTF
+#if defined HAS_BSD_WPRINTF
    retval = bsd_vsnwprintf(&str, size, format, ap);
 #elif defined(_WIN32)
+   /*
+    * _vsnwprintf() returns -1 on overflow, returns size when result fits
+    * exactly, and does not NUL terminate in those cases.
+    */
    retval = _vsnwprintf(str, size, format, ap);
+   if ((retval < 0 || retval >= size) && size > 0) {
+      str[size - 1] = L'\0';
+   }
 #else
-   retval = vswprintf(str, size, format, ap);
-#endif
-
    /*
-    * Linux glibc 2.0.x returns -1 and NUL terminates (which we shouldn't
-    * be linking against), but glibc 2.1.x follows c99 and returns
-    * characters that would have been written.
-    *
-    * In the case of Win32 and !HAS_BSD_PRINTF, we are using
-    * _vsnwprintf(), which returns -1 on overflow, returns size
-    * when result fits exactly, and does not NUL terminate in
-    * those cases.
+    * There is no standard vsnwprintf function.  vswprintf is close, but unlike
+    * vsnprintf, vswprintf always returns a negative value if truncation
+    * occurred.  Additionally, the state of the destination buffer on failure
+    * is not specified.  Although the C99 specification says that [v]swprintf
+    * should always NUL-terminate, glibc (as of 2.24) is non-conforming and
+    * seems to leave the final element untouched.
     */
-
-#if defined _WIN32 && !defined HAS_BSD_PRINTF
-   if ((retval < 0 || retval >= size) && size > 0) {
+   retval = vswprintf(str, size, format, ap);
+   if (retval < 0 && size > 0) {
       str[size - 1] = L'\0';
    }
 #endif
+
    if (retval >= size) {
       return -1;
    }
@@ -1281,115 +1290,103 @@ Str_ToUpper(char *string)  // IN
 #if 0
 
 /*
- * Unit tests. Compares our bsd_vs*printf code output to C-library
- * code output, where possible.
+ * Unit tests.
  */
 
-static Bool bCompare;
-
 #define FAIL(s) \
    do { \
       printf("FAIL: %s\n", s); \
       exit(1); \
    } while (0);
 
+
 static void
-PrintAndCheck(char *fmt,  // IN:
-              ...)        // IN:
+CheckPrintf(const char *expected,  // IN
+            const char *fmt,       // IN
+            ...)                   // IN
 {
-   char buf1[1024], buf2[1024];
+#if !defined HAS_BSD_PRINTF
+   NOT_TESTED_ONCE();
+#else
+   char buf[1024] = "";
    int count;
+   int expectedCount = strlen(expected);
    va_list args;
 
    va_start(args, fmt);
-   count = Str_Vsnprintf(buf1, sizeof buf1, fmt, args);
-   va_end(args);
-
-   if (count < 0) {
-      FAIL("PrintAndCheck new code count off");
-   }
-
-   va_start(args, fmt);
-
-#ifdef _WIN32
-   count = _vsnprintf(buf2, sizeof buf2, fmt, args);
-#else
-   count = vsnprintf(buf2, sizeof buf2, fmt, args);
-#endif
-
+   count = Str_Vsnprintf(buf, sizeof buf, fmt, args);
    va_end(args);
 
-   if (count < 0) {
-      FAIL("PrintAndCheck old code count off");
-   }
-
-   if (bCompare && (0 != strcmp(buf1, buf2))) {
-      printf("Format string: %s\n", fmt);
-      printf("Our code: %s\n", buf1);
-      printf("Sys code: %s\n", buf2);
+   // Verify there's a NUL somewhere since we print the buffer on failure.
+   VERIFY(buf[ARRAYSIZE(buf) - 1] == '\0');
 
-      FAIL("PrintAndCheck compare failed");
+   if (count == expectedCount && strcmp(buf, expected) == 0) {
+      // Success.
+      return;
    }
 
-   printf(buf1);
+   printf("%s\n", buf);
+   printf("Format string: %s\n", fmt);
+   printf("Expected count: %d\n", expectedCount);
+   printf("Expected output: %s\n", expected);
+   printf("Actual count: %d\n", count);
+   printf("Actual output: %s\n", buf);
+   FAIL("CheckPrintf");
+#endif
 }
 
+
 static void
-PrintAndCheckW(wchar_t *fmt, ...)
+CheckWPrintf(const wchar_t *expected, // IN
+             const wchar_t *fmt,      // IN
+             ...)                     // IN
 {
-   wchar_t buf1[1024], buf2[1024];
+#if !defined HAS_BSD_WPRINTF
+   NOT_TESTED_ONCE();
+#else
+   wchar_t buf[1024] = L"";
    int count;
+   int expectedCount = wcslen(expected);
    va_list args;
 
    va_start(args, fmt);
-   count = Str_Vsnwprintf(buf1, sizeof buf1, fmt, args);
+   count = Str_Vsnwprintf(buf, sizeof buf, fmt, args);
    va_end(args);
 
-   if (count < 0) {
-      FAIL("PrintAndCheckW new code count off");
-   }
-
-   va_start(args, fmt);
+   // Verify there's a NUL somewhere since we print the buffer on failure.
+   VERIFY(buf[ARRAYSIZE(buf) - 1] == L'\0');
 
-#ifdef _WIN32
-   count = _vsnwprintf(buf2, sizeof buf2, fmt, args);
-#else
-   count = vswprintf(buf2, sizeof buf2, fmt, args);
-#endif
-
-   va_end(args);
-
-   if (count < 0) {
-      FAIL("PrintAndCheckW old code count off");
+   if (count == expectedCount && wcscmp(buf, expected) == 0) {
+      // Success.
+      return;
    }
 
-   if (bCompare && (0 != wcscmp(buf1, buf2))) {
-      printf("Format string: %S", fmt);
-      printf("Our code: %S", buf1);
-      printf("Sys code: %S", buf2);
-
-      FAIL("PrintAndCheckW compare failed");
-   }
-
-#ifndef _WIN32
-   printf("%S", buf1);
-#endif // _WIN32
+   /*
+    * %S isn't standard, and since we use the system printf here, we must use
+    * %ls instead.
+    */
+   printf("%ls\n", buf);
+   printf("Format string: %ls\n", fmt);
+   printf("Expected count: %d\n", expectedCount);
+   printf("Expected output: %ls\n", expected);
+   printf("Actual count: %d\n", count);
+   printf("Actual output: %ls\n", buf);
+   FAIL("CheckWPrintf");
+#endif
 }
 
+
 void
 Str_UnitTests(void)
 {
    char buf[1024];
    wchar_t bufw[1024];
    int count;
+   const void *p = (void*) 0xFEEDFACE;
    int32 num1 = 0xDEADBEEF;
    int32 num2 = 0x927F82CD;
    int64 num3 = CONST64U(0xCAFEBABE42439021);
-#ifdef _WIN32
-   double num4 = 5.1923843;
-   double num5 = 0.000482734;
-   double num6 = 8274102.3872;
-#endif
+   const double d[] = { 5.0, 2017.0, 0.000482734, 8274102.3872 };
    int numChars;
    char empty[1] = {'\0'};
    wchar_t wempty[1] = {L'\0'};
@@ -1470,76 +1467,108 @@ Str_UnitTests(void)
       FAIL("Failed 'n' arg test - numChars (W)");
    }
 
-   bCompare = TRUE;
-
    // simple
-   PrintAndCheck("hello\n");
-   PrintAndCheckW(L"hello\n");
+   CheckPrintf("hello", "hello");
+   CheckWPrintf(L"hello", L"hello");
 
    // string arguments
-   PrintAndCheck("whazz %s up %S doc\n", "hello", L"hello");
-   PrintAndCheckW(L"whazz %s up %S doc\n", L"hello", "hello");
+   CheckPrintf("whazz hello up hello doc",
+               "whazz %s up %S doc", "hello", L"hello");
+   CheckWPrintf(L"whazz hello up hello doc",
+                L"whazz %s up %S doc", "hello", L"hello");
 
    // character arguments
-   PrintAndCheck("whazz %c up %C doc\n", 'a', L'a');
-   PrintAndCheckW(L"whazz %c up %C doc\n", L'a', 'a');
+   CheckPrintf("whazz a up a doc",
+               "whazz %c up %C doc", 'a', L'a');
+   CheckWPrintf(L"whazz a up a doc",
+                L"whazz %c up %C doc", 'a', L'a');
 
    // 32-bit integer arguments
-   PrintAndCheck("%d %i %o %u %x %X\n", num1, num1, num1, num1, num1,
-                 num1);
-   PrintAndCheckW(L"%d %i %o %u %x %X\n", num1, num1, num1, num1, num1,
-                  num1);
+   CheckPrintf("-559038737 -559038737 33653337357 3735928559 deadbeef DEADBEEF",
+               "%d %i %o %u %x %X", num1, num1, num1, num1, num1, num1);
+   CheckWPrintf(L"-559038737 -559038737 33653337357 3735928559 deadbeef DEADBEEF",
+                L"%d %i %o %u %x %X", num1, num1, num1, num1, num1, num1);
 
    // 'p' argument
-   bCompare = FALSE;
-   PrintAndCheck("%p\n", buf);
-   PrintAndCheckW(L"%p\n", buf);
-   bCompare = TRUE;
+   // XXX: bsd_vsnwprintf behaves differently from bsd_vsnprintf.
+   CheckPrintf("FEEDFACE", "%p", p);
+   CheckWPrintf(L"feedface", L"%p", p);
 
    // 64-bit
-   bCompare = FALSE;
-   PrintAndCheck("%LX %llX %qX\n", num3, num3, num3);
-   PrintAndCheckW(L"%LX %llX %qX\n", num3, num3, num3);
-   bCompare = TRUE;
+   CheckPrintf("CAFEBABE42439021",
+               "%LX", num3);
+   CheckWPrintf(L"CAFEBABE42439021",
+                L"%LX", num3);
+   CheckPrintf("CAFEBABE42439021",
+               "%llX", num3);
+   CheckWPrintf(L"CAFEBABE42439021",
+                L"%llX", num3);
+   CheckPrintf("CAFEBABE42439021",
+               "%qX", num3);
+   CheckWPrintf(L"CAFEBABE42439021",
+                L"%qX", num3);
+   CheckPrintf("CAFEBABE42439021",
+               "%I64X", num3);
+   CheckWPrintf(L"CAFEBABE42439021",
+                L"%I64X", num3);
 
-   // more 64-bit
-#ifdef _WIN32
-   PrintAndCheck("%I64X\n", num3);
-   PrintAndCheckW(L"%I64X\n", num3);
-#else
-   PrintAndCheck("%LX\n", num3);
-   PrintAndCheckW(L"%LX\n", num3);
-#endif
-
-#ifdef _WIN32 // exponent digits printed differs vs. POSIX
    // floating-point
-   PrintAndCheck("%e %E %f %g %G\n", num4, num5, num6);
-   PrintAndCheckW(L"%e %E %f %g %G\n", num4, num5, num6);
-#endif
+   {
+      const char *expected[] = {
+         "5.000000e+00 5.000000E+00 5.000000 5 5",
+         "2.017000e+03 2.017000E+03 2017.000000 2017 2017",
+         "4.827340e-04 4.827340E-04 0.000483 0.000482734 0.000482734",
+         "8.274102e+06 8.274102E+06 8274102.387200 8.2741e+06 8.2741E+06",
+      };
+      // XXX: bsd_vsnwprintf behaves differently from bsd_vsnprintf.
+      const wchar_t *expectedW[] = {
+         L"5.000000e+000 5.000000E+000 5.000000 5 5",
+         L"2.017000e+003 2.017000E+003 2017.000000 2017 2017",
+         L"4.827340e-004 4.827340E-004 0.000483 0.000482734 0.000482734",
+         L"8.274102e+006 8.274102E+006 8274102.387200 8.2741e+006 8.2741E+006",
+      };
+
+      size_t i;
+
+      ASSERT_ON_COMPILE(ARRAYSIZE(d) == ARRAYSIZE(expected));
+      ASSERT_ON_COMPILE(ARRAYSIZE(d) == ARRAYSIZE(expectedW));
+
+      for (i = 0; i < ARRAYSIZE(d); i++) {
+         CheckPrintf(expected[i],
+                     "%e %E %f %g %G", d[i], d[i], d[i], d[i], d[i]);
+         CheckWPrintf(expectedW[i],
+                      L"%e %E %f %g %G", d[i], d[i], d[i], d[i], d[i]);
+      }
+   }
 
    // positional arguments
-   bCompare = FALSE;
-   PrintAndCheck("%3$LX %1$x %2$x\n", num1, num2, num3);
-   PrintAndCheckW(L"%3$LX %1$x %2$x\n", num1, num2, num3);
-   bCompare = TRUE;
+   CheckPrintf("CAFEBABE42439021 deadbeef 927f82cd",
+               "%3$LX %1$x %2$x", num1, num2, num3);
+   CheckWPrintf(L"CAFEBABE42439021 deadbeef 927f82cd",
+                L"%3$LX %1$x %2$x", num1, num2, num3);
 
-#ifdef _WIN32 // exponent digits printed differs vs. POSIX
    // width and precision
-   PrintAndCheck("%15.1g %20.2f %*.*f\n", num6, num6, 15, 3, num6);
-   PrintAndCheckW(L"%15.1g %20.2f %*.*f\n", num6, num6, 15, 3, num6);
-#endif
+   // XXX: bsd_vsnwprintf behaves differently from bsd_vsnprintf.
+   CheckPrintf("          8e+06           8274102.39     8274102.387",
+               "%15.1g %20.2f %*.*f", d[3], d[3], 15, 3, d[3]);
+   CheckWPrintf(L"         8e+006           8274102.39     8274102.387",
+                L"%15.1g %20.2f %*.*f", d[3], d[3], 15, 3, d[3]);
 
-#ifdef _WIN32 // exponent digits printed differs vs. POSIX
    // flags
-   PrintAndCheck("%-15e %+f %015g\n", num4, num5, num6);
-   PrintAndCheckW(L"%-15e %+f %015g\n", num4, num5, num6);
-#endif
+   // XXX: bsd_vsnwprintf behaves differently from bsd_vsnprintf.
+   CheckPrintf("5.000000e+00    +0.000483 000008.2741e+06",
+               "%-15e %+f %015g", d[0], d[2], d[3]);
+   CheckWPrintf(L"5.000000e+000   +0.000483 00008.2741e+006",
+                L"%-15e %+f %015g", d[0], d[2], d[3]);
 
-#ifdef _WIN32 // exponent digits printed differs vs. POSIX
    // more flags
-   PrintAndCheck("%#X %#E %#G\n", num1, num1, num1);
-   PrintAndCheckW(L"%#X %#E %#G\n", num1, num1, num1);
-#endif
+   // XXX: bsd_vsnwprintf behaves differently from bsd_vsnprintf.
+   CheckPrintf("0XDEADBEEF 5.000000E+00 2017.00",
+               "%#X %#E %#G", num1, d[0], d[1]);
+   CheckWPrintf(L"0XDEADBEEF 5.000000E+000 2017.00",
+                L"%#X %#E %#G", num1, d[0], d[1]);
+
+   printf("%s succeeded.\n", __FUNCTION__);
 }
 
 #endif // 0