[PATCH v2 2/2] Various improvements and fixes

[Stefan Rentsch]

From: Stefan Rentsch

* Fixes issues reported by buildbot
* Use of _vscprintf instead of vsprintf(NULL, ...)
* Fixes unnecessary reallocations caused by incorrect capacity checks
* String length parameter -> more use-cases
---
 dlls/msvcrt/tests/winestring.c | 117 ++++++++++++++++++++++-----------
 include/wine/string.h          |  79 +++++++++++++---------
 2 files changed, 126 insertions(+), 70 deletions(-)

diff --git a/dlls/msvcrt/tests/winestring.c b/dlls/msvcrt/tests/winestring.c
index b296818408e..6da73ca6a3e 100644
--- a/dlls/msvcrt/tests/winestring.c
+++ b/dlls/msvcrt/tests/winestring.c
@@ -19,8 +19,10 @@
 #include "wine/string.h"
 #include "wine/test.h"

-static const WCHAR *dummy_text_w = L"foo BAR /BAZ/";
-static const char  *dummy_text_a =  "foo BAR /BAZ/";
+static const WCHAR *dummy_text_w      = L"foo BAR /BAZ/";
+static const WCHAR *dummy_text_w_part = L"foo BAR /";
+static const char  *dummy_text_a      =  "foo BAR /BAZ/";
+static const char  *dummy_text_a_part =  "foo BAR /";

static void test_init_append(void)
 {
@@ -31,25 +33,25 @@ static void test_init_append(void)
         ok(WineAStrInit(&str, MAX_PATH - 1) == S_OK, "@init\n");

/* string concat */
-        ok(WineAStrAppendA(&str, "Hello world") == S_OK, "@append\n");
-        WineAStrAppendA(&str, "Hello world");
-        WineAStrAppendA(&str, "Hello world");
-        ok(str.length == 11 * 3, "len=%ld\n", str.length);
-        ok(strlen(str.data) == str.length, "len=%ld\n", str.length);
+        ok(WineAStrAppend(&str, "Hello world") == S_OK, "@append\n");
+        WineAStrAppend(&str, "Hello world");
+        WineAStrAppend(&str, "Hello world");
+        ok(str.length == 11 * 3, "len=%zu\n", str.length);
+        ok(strlen(str.data) == str.length, "len=%zu\n", str.length);
         ok(str.capacity >= str.length, "cap=%ld\n", str.capacity);

/* resize without shrink */
         WineAStrResize(&str, str.length + 10);
-        ok(str.length == 11 * 3, "len=%ld\n", str.length);
-        ok(strlen(str.data) == str.length, "len=%ld\n", str.length);
+        ok(str.length == 11 * 3, "len=%zu\n", str.length);
+        ok(strlen(str.data) == str.length, "len=%zu\n", str.length);

/* resize with shrink */
         for (i = 3; i < 100; ++i)
-            WineAStrAppendA(&str, "Hello world");
-        ok(str.length == 11 * 100, "len=%ld\n", str.length);
+            WineAStrAppend(&str, "Hello world");
+        ok(str.length == 11 * 100, "len=%zu\n", str.length);
         WineAStrResize(&str, MAX_PATH - 1);
-        ok(str.length == MAX_PATH - 1, "len=%ld\n", str.length);
-        ok(strlen(str.data) == str.length, "len=%ld\n", str.length);
+        ok(str.length == MAX_PATH - 1, "len=%zu\n", str.length);
+        ok(strlen(str.data) == str.length, "len=%zu\n", str.length);

WineAStrFree(&str);
     }
@@ -59,25 +61,25 @@ static void test_init_append(void)
         ok(WineWStrInit(&str, MAX_PATH - 1) == S_OK, "@init\n");

/* string concat */
-        ok(WineWStrAppendW(&str, L"Hello world") == S_OK, "@append\n");
-        WineWStrAppendW(&str, L"Hello world");
-        WineWStrAppendW(&str, L"Hello world");
-        ok(str.length == 11 * 3, "len=%ld\n", str.length);
-        ok(lstrlenW(str.data) == str.length, "len=%ld\n", str.length);
+        ok(WineWStrAppend(&str, L"Hello world") == S_OK, "@append\n");
+        WineWStrAppend(&str, L"Hello world");
+        WineWStrAppend(&str, L"Hello world");
+        ok(str.length == 11 * 3, "len=%zu\n", str.length);
+        ok(lstrlenW(str.data) == str.length, "len=%zu\n", str.length);
         ok(str.capacity >= str.length, "cap=%ld\n", str.capacity);

/* resize without shrink */
         WineWStrResize(&str, str.length + 10);
-        ok(str.length == 11 * 3, "len=%ld\n", str.length);
-        ok(lstrlenW(str.data) == str.length, "len=%ld\n", str.length);
+        ok(str.length == 11 * 3, "len=%zu\n", str.length);
+        ok(lstrlenW(str.data) == str.length, "len=%zu\n", str.length);

/* resize with shrink */
         for (i = 3; i < 100; ++i)
-            WineWStrAppendW(&str, L"Hello world");
-        ok(str.length == 11 * 100, "len=%ld\n", str.length);
+            WineWStrAppend(&str, L"Hello world");
+        ok(str.length == 11 * 100, "len=%zu\n", str.length);
         WineWStrResize(&str, MAX_PATH - 1);
-        ok(str.length == MAX_PATH - 1, "len=%ld\n", str.length);
-        ok(lstrlenW(str.data) == str.length, "len=%ld\n", str.length);
+        ok(str.length == MAX_PATH - 1, "len=%zu\n", str.length);
+        ok(lstrlenW(str.data) == str.length, "len=%zu\n", str.length);

WineWStrFree(&str);
     }
@@ -91,18 +93,18 @@ static void test_replace_heap(void)
         char *new_heap;
         WINEASTR str;
         WineAStrInit(&str, alloc_count);
-        WineAStrAppendA(&str, "data to replace");
+        WineAStrAppend(&str, "data to replace");

new_heap = heap_alloc(alloc_count * sizeof(char));
         strcpy(new_heap, dummy_text_a);
         WineAStrReplaceHeap(&str, &new_heap);
         ok(new_heap == NULL, "p=%p\n", new_heap);
-        ok(str.length == strlen(dummy_text_a), "len=%ld\n", str.length);
+        ok(str.length == strlen(dummy_text_a), "len=%zu\n", str.length);
         ok(strcmp(str.data, dummy_text_a) == 0, "@strcmp\n");

new_heap = NULL;
         WineAStrReplaceHeap(&str, &new_heap);
-        ok(strlen(str.data) == 0, "len=%ld\n", str.length);
+        ok(strlen(str.data) == 0, "len=%zu\n", str.length);

WineAStrFree(&str);
     }
@@ -111,18 +113,18 @@ static void test_replace_heap(void)
         WCHAR *new_heap;
         WINEWSTR str;
         WineWStrInit(&str, alloc_count);
-        WineWStrAppendW(&str, L"data to replace");
+        WineWStrAppend(&str, L"data to replace");

new_heap = heap_alloc(alloc_count * sizeof(WCHAR));
         lstrcpyW(new_heap, dummy_text_w);
         WineWStrReplaceHeap(&str, &new_heap);
         ok(new_heap == NULL, "p=%p\n", new_heap);
-        ok(str.length == lstrlenW(dummy_text_w), "len=%ld\n", str.length);
+        ok(str.length == lstrlenW(dummy_text_w), "len=%zu\n", str.length);
         ok(lstrcmpW(str.data, dummy_text_w) == 0, "@lstrcmpW\n");

new_heap = NULL;
         WineWStrReplaceHeap(&str, &new_heap);
-        ok(lstrlenW(str.data) == 0, "len=%ld\n", str.length);
+        ok(lstrlenW(str.data) == 0, "len=%zu\n", str.length);

WineWStrFree(&str);
     }
@@ -135,13 +137,13 @@ static void test_append_convert(void)
         WINEASTR str;
         WineAStrInit(&str, 0);

-        WineAStrAppendW(&str, CP_ACP, dummy_text_w);
-        ok(strlen(str.data) == str.length, "len=%ld\n", str.length);
+        WineAStrAppendW(&str, CP_ACP, dummy_text_w, -1);
+        ok(strlen(str.data) == str.length, "len=%zu\n", str.length);
         ok(strcmp(str.data, dummy_text_a) == 0, "@strcmp\n");

/* repeat. double length. */
-        WineAStrAppendW(&str, CP_ACP, dummy_text_w);
-        ok(str.length == strlen(dummy_text_a) * 2, "len=%ld\n", str.length);
+        WineAStrAppendW(&str, CP_ACP, dummy_text_w, -1);
+        ok(str.length == strlen(dummy_text_a) * 2, "len=%zu\n", str.length);

WineAStrFree(&str);
     }
@@ -150,13 +152,47 @@ static void test_append_convert(void)
         WINEWSTR str;
         WineWStrInit(&str, 0);

-        WineWStrAppendA(&str, CP_ACP, dummy_text_a);
-        ok(lstrlenW(str.data) == str.length, "len=%ld\n", str.length);
+        WineWStrAppendA(&str, CP_ACP, dummy_text_a, -1);
+        ok(lstrlenW(str.data) == str.length, "len=%zu\n", str.length);
         ok(lstrcmpW(str.data, dummy_text_w) == 0, "@lstrcmpW\n");

/* repeat. double length. */
-        WineWStrAppendA(&str, CP_ACP, dummy_text_a);
-        ok(str.length == lstrlenW(dummy_text_w) * 2, "len=%ld\n", str.length);
+        WineWStrAppendA(&str, CP_ACP, dummy_text_a, -1);
+        ok(str.length == lstrlenW(dummy_text_w) * 2, "len=%zu\n", str.length);
+
+        WineWStrFree(&str);
+    }
+}
+
+static void test_append_limited(void)
+{
+    {
+        /* ANSI string: append with specified length */
+        WINEASTR str;
+        WineAStrInit(&str, 0);
+
+        ok(WineAStrAppendA(&str, dummy_text_a, 9) == S_OK, "@appendN\n");
+        ok(str.length == 9, "len=%zu\n", str.length);
+        ok(strcmp(str.data, dummy_text_a_part) == 0, "@strcmp\n");
+
+        ok(WineAStrAppendW(&str, CP_ACP, dummy_text_w, 9) == S_OK, "@appendN\n");
+        ok(str.length == 18, "len=%zu, %s\n", str.length, str.data);
+        ok(strcmp(str.data + 9, dummy_text_a_part) == 0, "@strcmp\n");
+
+        WineAStrFree(&str);
+    }
+    {
+        /* Wide string: append with specified length */
+        WINEWSTR str;
+        WineWStrInit(&str, 0);
+
+        ok(WineWStrAppendW(&str, dummy_text_w, 9) == S_OK, "@appendN\n");
+        ok(str.length == 9, "len=%zu\n", str.length);
+        ok(lstrcmpW(str.data, dummy_text_w_part) == 0, "@lstrcmpW\n");
+
+        ok(WineWStrAppendA(&str, CP_ACP, dummy_text_a, 9) == S_OK, "@appendN\n");
+        ok(str.length == 18, "len=%zu\n", str.length);
+        ok(lstrcmpW(str.data + 9, dummy_text_w_part) == 0, "@lstrcmpW\n");

WineWStrFree(&str);
     }
@@ -170,7 +206,7 @@ static void test_append_formatted(void)
         WineAStrInit(&str, 0);

ok(WineAStrAppendFmt(&str, "TEST %d %c+", 321, 'X') == S_OK, "@appendFmt\n");
-        ok(strcmp(str.data, "TEST 321 X+") == 0, "@strcmp, len=%ld\n", str.length);
+        ok(strcmp(str.data, "TEST 321 X+") == 0, "@strcmp, len=%zu\n", str.length);

WineAStrFree(&str);
     }
@@ -180,7 +216,7 @@ static void test_append_formatted(void)
         WineWStrInit(&str, 0);

ok(WineWStrAppendFmt(&str, L"TEST %d %c+", 321, L'X') == S_OK, "@appendFmt\n");
-        ok(lstrcmpW(str.data, L"TEST 321 X+") == 0, "@lstrcmpW, len=%ld\n", str.length);
+        ok(lstrcmpW(str.data, L"TEST 321 X+") == 0, "@lstrcmpW, len=%zu\n", str.length);

WineWStrFree(&str);
     }
@@ -191,5 +227,6 @@ START_TEST(winestring)
     test_init_append();
     test_replace_heap();
     test_append_convert();
+    test_append_limited();
     test_append_formatted();
 }
diff --git a/include/wine/string.h b/include/wine/string.h
index 15a76fcd2fa..66a834bd213 100644
--- a/include/wine/string.h
+++ b/include/wine/string.h
@@ -33,6 +33,9 @@
 #endif

+#define WineAStrAppend(str, data) WineAStrAppendA(str, data, -1)
+#define WineWStrAppend(str, data) WineWStrAppendW(str, data, -1)
+
 /** Dynamic container for ANSI strings */
 typedef struct _WINEASTR {
     char *data;      /**< C-string. The address may change due to realloc. */
@@ -41,7 +44,7 @@ typedef struct _WINEASTR {
 } WINEASTR;

/** One-time dynamic container initialization
- * @param capacity Amount of characters to preallocate
+ * @param capacity Amount of characters to preallocate. Higher values result in less reallocations.
  */
 static HRESULT WineAStrInit(WINEASTR *str, size_t capacity)
 {
@@ -76,13 +79,13 @@ static void WineAStrReplaceHeap(WINEASTR *str, char **src_data)
     }
 }

-/** Expand (realloc) or trim the container data
+/** Expand (realloc) or trim the container data. Doubles the specified capacity upon growth.
  * @param capacity Minimal amount of characters to hold.
  */
 static HRESULT WineAStrResize(WINEASTR *str, size_t capacity)
 {
     if (capacity > str->capacity) {
-        char *new_ptr = heap_realloc(str->data, (capacity + 1) * sizeof(char));
+        char *new_ptr = heap_realloc(str->data, (capacity * 2 + 1) * sizeof(char));
         if (!new_ptr)
             return E_OUTOFMEMORY;

@@ -96,7 +99,7 @@ static HRESULT WineAStrResize(WINEASTR *str, size_t capacity)
     return S_OK;
 }

-static HRESULT WineAStrAppendA(WINEASTR *str, const char *data)
+static HRESULT WineAStrAppendA(WINEASTR *str, const char *data, int n)
 {
     HRESULT hr;
     size_t len;
@@ -104,35 +107,43 @@ static HRESULT WineAStrAppendA(WINEASTR *str, const char *data)
     if (!data[0])
         return S_OK;

-    len = strlen(data);
-    hr = WineAStrResize(str, (str->length + len) * 2);
+    len = n < 0 ? strlen(data) : n;
+    hr = WineAStrResize(str, str->length + len);
     if (hr != S_OK)
         return hr;

-    memcpy(&str->data[str->length], data, (len + 1) * sizeof(char));
+    memcpy(&str->data[str->length], data, len * sizeof(char));
     str->length += len;
+    str->data[str->length] = 0;
     return S_OK;
 }

-/** Convert and append PWSTR (WCHAR*) */
-static HRESULT WineAStrAppendW(WINEASTR *str, UINT codepage, const WCHAR *data)
+/** Convert and append PWSTR (WCHAR*)
+ * @param n Amount of characters to convert. -1 to convert until a null character.is reached
+ */
+static HRESULT WineAStrAppendW(WINEASTR *str, UINT codepage, const WCHAR *data, int n)
 {
     HRESULT hr;
-    size_t len;
+    int len;

if (!data[0])
         return S_OK;

-    len = WideCharToMultiByte(codepage, 0, data, -1, NULL, 0, NULL, NULL);
-    hr = WineAStrResize(str, (str->length + len) * 2);
+    len = WideCharToMultiByte(codepage, 0, data, n, NULL, 0, NULL, NULL);
+    hr = WineAStrResize(str, str->length + len);
     if (hr != S_OK)
         return hr;

-    len = WideCharToMultiByte(codepage, 0, data, -1, &str->data[str->length], str->capacity + 1, NULL, NULL);
+    len = WideCharToMultiByte(codepage, 0, data, n, &str->data[str->length], str->capacity + 1, NULL, NULL);
     if (len <= 0)
         return HRESULT_FROM_WIN32(GetLastError());

+    /* The null character might or might not be written */
     str->length += len - 1;
+    if (str->data[str->length] != 0) {
+        str->length++;
+        str->data[str->length] = 0;
+    }
     return S_OK;
 }

@@ -144,13 +155,13 @@ static HRESULT WINAPIV __WINE_PRINTF_ATTR(2,3) WineAStrAppendFmt(WINEASTR *str,
     va_list valist;

va_start(valist, fmt);
-    len = vsprintf(NULL, fmt, valist);
+    len = _vscprintf(fmt, valist);
     if (len < 0) {
         hr = E_UNEXPECTED;
         goto end;
     }

-    hr = WineAStrResize(str, (str->length + len) * 2);
+    hr = WineAStrResize(str, str->length + len);
     if (hr != S_OK)
         goto end;

@@ -172,7 +183,7 @@ typedef struct _WINEWSTR {
 } WINEWSTR;

/** One-time dynamic container initialization
- * @param capacity Amount of characters to preallocate
+ * @param capacity Amount of characters to preallocate. Higher values result in less reallocations.
  */
 static HRESULT WineWStrInit(WINEWSTR *str, size_t capacity)
 {
@@ -207,13 +218,13 @@ static void WineWStrReplaceHeap(WINEWSTR *str, WCHAR **src_data)
     }
 }

-/** Expand (realloc) or trim the container data
+/** Expand (realloc) or trim the container data. Doubles the specified capacity upon growth.
  * @param capacity Minimal amount of characters to hold.
  */
 static HRESULT WineWStrResize(WINEWSTR *str, size_t capacity)
 {
     if (capacity > str->capacity) {
-        WCHAR *new_ptr = heap_realloc(str->data, (capacity + 1) * sizeof(WCHAR));
+        WCHAR *new_ptr = heap_realloc(str->data, (capacity * 2 + 1) * sizeof(WCHAR));
         if (!new_ptr)
             return E_OUTOFMEMORY;

@@ -227,7 +238,7 @@ static HRESULT WineWStrResize(WINEWSTR *str, size_t capacity)
     return S_OK;
 }

-static HRESULT WineWStrAppendW(WINEWSTR *str, const WCHAR *data)
+static HRESULT WineWStrAppendW(WINEWSTR *str, const WCHAR *data, int n)
 {
     HRESULT hr;
     size_t len;
@@ -235,35 +246,43 @@ static HRESULT WineWStrAppendW(WINEWSTR *str, const WCHAR *data)
     if (!data[0])
         return S_OK;

-    len = lstrlenW(data);
-    hr = WineWStrResize(str, (str->length + len) * 2);
+    len = n < 0 ? lstrlenW(data) : n;
+    hr = WineWStrResize(str, str->length + len);
     if (hr != S_OK)
         return hr;

-    memcpy(&str->data[str->length], data, (len + 1) * sizeof(WCHAR));
+    memcpy(&str->data[str->length], data, len * sizeof(WCHAR));
     str->length += len;
+    str->data[str->length] = 0;
     return S_OK;
 }

-/** Convert and append PSTR (char*) */
-static HRESULT WineWStrAppendA(WINEWSTR *str, UINT codepage, const char *data)
+/** Convert and append PSTR (char*)
+ * @param n Amount of characters to convert. -1 to convert until a null character.is reached
+ */
+static HRESULT WineWStrAppendA(WINEWSTR *str, UINT codepage, const char *data, int n)
 {
     HRESULT hr;
-    size_t len;
+    int len;

if (!data[0])
         return S_OK;

-    len = MultiByteToWideChar(codepage, 0, data, -1, NULL, 0);
-    hr = WineWStrResize(str, (str->length + len) * 2);
+    len = MultiByteToWideChar(codepage, 0, data, n, NULL, 0);
+    hr = WineWStrResize(str, str->length + len);
     if (hr != S_OK)
         return hr;

-    len = MultiByteToWideChar(codepage, 0, data, -1, &str->data[str->length], str->capacity + 1);
+    len = MultiByteToWideChar(codepage, 0, data, n, &str->data[str->length], str->capacity + 1);
     if (len <= 0)
         return HRESULT_FROM_WIN32(GetLastError());

+    /* The null character might or might not be written */
     str->length += len - 1;
+    if (str->data[str->length] != 0) {
+        str->length++;
+        str->data[str->length] = 0;
+    }
     return S_OK;
 }

@@ -275,13 +294,13 @@ static HRESULT WINAPIV WineWStrAppendFmt(WINEWSTR *str, const WCHAR *fmt, ...)
     va_list valist;

va_start(valist, fmt);
-    len = vswprintf(NULL, -1, fmt, valist);
+    len = _vscwprintf(fmt, valist);
     if (len < 0) {
         hr = E_UNEXPECTED;
         goto end;
     }

-    hr = WineWStrResize(str, (str->length + len) * 2);
+    hr = WineWStrResize(str, str->length + len);
     if (hr != S_OK)
         goto end;