[PATCH 0/1] MR910: msvcrt: Implement dynamic string container
Many code sections still use fixed-size arrays on stack - or even worse - `strcat` in combination with strings of unknown length. This MR/PR provides general purpose Wine-internal functions for safer string handling. The functions are currently header-only to not pollute msvcrt exports. The API was written for an upcoming shell32/shlexec patch (WIP) to properly fix possible stack corruptions due to out-of-bounds writes for unexpected long paths or URLs. This is a dynamic container inspired by C++ std::string / std::vector with wide/narrow conversion and formatted text. Growth factor = 2 for simplicity. Differences to `STRING`/`UNICODE_STRING` (`RtlCreateUnicodeString` and similar): * No USHORT length limitation * Actual dynamic resizable container * String formatting and conversion support * Short and consistent API naming New test added to msvcrt: `winestring` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/910
From: Stefan Rentsch <et14rest(a)gmail.com> The API was written for an upcoming shell32/shlexec patch to properly fix possible stack corruptions due to out-of-bounds writes. This is a dynamic container inspired by C++ std::string / std::vector with wide/narrow conversion and formatted text. Growth factor = 2 for simplicity. --- dlls/msvcrt/tests/Makefile.in | 3 +- dlls/msvcrt/tests/winestring.c | 195 +++++++++++++++++++++ include/Makefile.in | 1 + include/wine/string.h | 298 +++++++++++++++++++++++++++++++++ 4 files changed, 496 insertions(+), 1 deletion(-) create mode 100644 dlls/msvcrt/tests/winestring.c create mode 100644 include/wine/string.h diff --git a/dlls/msvcrt/tests/Makefile.in b/dlls/msvcrt/tests/Makefile.in index 1db6da2a0ad..572bb9b9b11 100644 --- a/dlls/msvcrt/tests/Makefile.in +++ b/dlls/msvcrt/tests/Makefile.in @@ -14,4 +14,5 @@ C_SRCS = \ scanf.c \ signal.c \ string.c \ - time.c + time.c \ + winestring.c diff --git a/dlls/msvcrt/tests/winestring.c b/dlls/msvcrt/tests/winestring.c new file mode 100644 index 00000000000..b296818408e --- /dev/null +++ b/dlls/msvcrt/tests/winestring.c @@ -0,0 +1,195 @@ +/* + * Copyright (C) the Wine project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#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 void test_init_append(void) +{ + size_t i; + { + /* ANSI string: grow and shrink */ + WINEASTR str; + 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(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); + + /* resize with shrink */ + for (i = 3; i < 100; ++i) + WineAStrAppendA(&str, "Hello world"); + ok(str.length == 11 * 100, "len=%ld\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); + + WineAStrFree(&str); + } + { + /* Wide string: grow and shrink */ + WINEWSTR str; + 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(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); + + /* resize with shrink */ + for (i = 3; i < 100; ++i) + WineWStrAppendW(&str, L"Hello world"); + ok(str.length == 11 * 100, "len=%ld\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); + + WineWStrFree(&str); + } +} + +static void test_replace_heap(void) +{ + const size_t alloc_count = 100; + { + /* ANSI string */ + char *new_heap; + WINEASTR str; + WineAStrInit(&str, alloc_count); + WineAStrAppendA(&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(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); + + WineAStrFree(&str); + } + { + /* Unicode string */ + WCHAR *new_heap; + WINEWSTR str; + WineWStrInit(&str, alloc_count); + WineWStrAppendW(&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(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); + + WineWStrFree(&str); + } +} + +static void test_append_convert(void) +{ + { + /* ANSI string: append with width conversion */ + WINEASTR str; + WineAStrInit(&str, 0); + + WineAStrAppendW(&str, CP_ACP, dummy_text_w); + ok(strlen(str.data) == str.length, "len=%ld\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); + + WineAStrFree(&str); + } + { + /* Wide string: append with width conversion */ + WINEWSTR str; + WineWStrInit(&str, 0); + + WineWStrAppendA(&str, CP_ACP, dummy_text_a); + ok(lstrlenW(str.data) == str.length, "len=%ld\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); + + WineWStrFree(&str); + } +} + +static void test_append_formatted(void) +{ + { + /* ANSI string: append with formatting */ + WINEASTR str; + 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); + + WineAStrFree(&str); + } + { + /* Wide string: append with formatting */ + WINEWSTR str; + 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); + + WineWStrFree(&str); + } +} + +START_TEST(winestring) +{ + test_init_append(); + test_replace_heap(); + test_append_convert(); + test_append_formatted(); +} diff --git a/include/Makefile.in b/include/Makefile.in index 1c04f9a298b..f495b50fabd 100644 --- a/include/Makefile.in +++ b/include/Makefile.in @@ -846,6 +846,7 @@ SOURCES = \ wine/schrpc.idl \ wine/server.h \ wine/server_protocol.h \ + wine/string.h \ wine/strmbase.h \ wine/svcctl.idl \ wine/test.h \ diff --git a/include/wine/string.h b/include/wine/string.h new file mode 100644 index 00000000000..15a76fcd2fa --- /dev/null +++ b/include/wine/string.h @@ -0,0 +1,298 @@ +/* + * Copyright (C) the Wine project + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WINE_STRING_H +#define __WINE_WINE_STRING_H + +#include <assert.h> +#include <stdio.h> /* va_list */ +#include <windef.h> /* HRESULT status codes */ +#include <wine/heap.h> +#include <winnls.h> /* MultiByteToWideChar */ + +#if !defined(__WINE_USE_MSVCRT) || defined(__MINGW32__) || defined(__GNUC__) +# define __WINE_PRINTF_ATTR(fmt,args) __attribute__((format (printf,fmt,args))) +/* there is no wprintf version */ +#else +# define __WINE_PRINTF_ATTR(fmt,args) +#endif + + +/** Dynamic container for ANSI strings */ +typedef struct _WINEASTR { + char *data; /**< C-string. The address may change due to realloc. */ + size_t length; /**< index of '\0' terminator */ + size_t capacity; /**< allocated indices without terminator */ +} WINEASTR; + +/** One-time dynamic container initialization + * @param capacity Amount of characters to preallocate + */ +static HRESULT WineAStrInit(WINEASTR *str, size_t capacity) +{ + memset(str, 0, sizeof(*str)); + if (!(str->data = heap_alloc((capacity + 1) * sizeof(char)))) + return E_OUTOFMEMORY; + + str->data[0] = 0; + str->capacity = capacity; + return S_OK; +} + +static void WineAStrFree(WINEASTR *str) +{ + heap_free(str->data); + memset(str, 0, sizeof(*str)); +} + +/** Frees the existing container and takes the ownership of the provided existing heap data. + * If *src_data is NULL, the container will be reinitialized with zero capacity. + * @param src_data Source address to takeover. The value is set to NULL on success. + */ +static void WineAStrReplaceHeap(WINEASTR *str, char **src_data) +{ + WineAStrFree(str); + if (*src_data) { + str->data = *src_data; + str->length = str->capacity = strlen(str->data); + *src_data = NULL; + } else { + WineAStrInit(str, 0); + } +} + +/** Expand (realloc) or trim the container data + * @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)); + if (!new_ptr) + return E_OUTOFMEMORY; + + str->data = new_ptr; + str->capacity = capacity; + } else if (capacity < str->length) { + /* Cut off string only */ + str->length = capacity; + str->data[capacity] = 0; + } + return S_OK; +} + +static HRESULT WineAStrAppendA(WINEASTR *str, const char *data) +{ + HRESULT hr; + size_t len; + + if (!data[0]) + return S_OK; + + len = strlen(data); + hr = WineAStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + return hr; + + memcpy(&str->data[str->length], data, (len + 1) * sizeof(char)); + str->length += len; + return S_OK; +} + +/** Convert and append PWSTR (WCHAR*) */ +static HRESULT WineAStrAppendW(WINEASTR *str, UINT codepage, const WCHAR *data) +{ + HRESULT hr; + size_t len; + + if (!data[0]) + return S_OK; + + len = WideCharToMultiByte(codepage, 0, data, -1, NULL, 0, NULL, NULL); + hr = WineAStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + return hr; + + len = WideCharToMultiByte(codepage, 0, data, -1, &str->data[str->length], str->capacity + 1, NULL, NULL); + if (len <= 0) + return HRESULT_FROM_WIN32(GetLastError()); + + str->length += len - 1; + return S_OK; +} + +/** String format and append to the container */ +static HRESULT WINAPIV __WINE_PRINTF_ATTR(2,3) WineAStrAppendFmt(WINEASTR *str, const char *fmt, ...) +{ + HRESULT hr; + int len; + va_list valist; + + va_start(valist, fmt); + len = vsprintf(NULL, fmt, valist); + if (len < 0) { + hr = E_UNEXPECTED; + goto end; + } + + hr = WineAStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + goto end; + + vsprintf(&str->data[str->length], fmt, valist); + str->length += len; + +end: + va_end(valist); + return hr; +} + +/* ====================================== */ + +/** Dynamic container for Unicode strings */ +typedef struct _WINEWSTR { + WCHAR *data; /**< C-string. The address may change due to realloc. */ + size_t length; /**< index of '\0' terminator */ + size_t capacity; /**< allocated indices without terminator */ +} WINEWSTR; + +/** One-time dynamic container initialization + * @param capacity Amount of characters to preallocate + */ +static HRESULT WineWStrInit(WINEWSTR *str, size_t capacity) +{ + memset(str, 0, sizeof(*str)); + if (!(str->data = heap_alloc((capacity + 1) * sizeof(WCHAR)))) + return E_OUTOFMEMORY; + + str->data[0] = 0; + str->capacity = capacity; + return S_OK; +} + +static void WineWStrFree(WINEWSTR *str) +{ + heap_free(str->data); + memset(str, 0, sizeof(*str)); +} + +/** Frees the existing container and takes the ownership of the provided existing heap data. + * If *src_data is NULL, the container will be reinitialized with zero capacity. + * @param src_data Pointer to source address to takeover. The value is set to NULL on success. + */ +static void WineWStrReplaceHeap(WINEWSTR *str, WCHAR **src_data) +{ + WineWStrFree(str); + if (*src_data) { + str->data = *src_data; + str->length = str->capacity = lstrlenW(str->data); + *src_data = NULL; + } else { + WineWStrInit(str, 0); + } +} + +/** Expand (realloc) or trim the container data + * @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)); + if (!new_ptr) + return E_OUTOFMEMORY; + + str->data = new_ptr; + str->capacity = capacity; + } else if (capacity < str->length) { + /* Cut off string only */ + str->length = capacity; + str->data[capacity] = 0; + } + return S_OK; +} + +static HRESULT WineWStrAppendW(WINEWSTR *str, const WCHAR *data) +{ + HRESULT hr; + size_t len; + + if (!data[0]) + return S_OK; + + len = lstrlenW(data); + hr = WineWStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + return hr; + + memcpy(&str->data[str->length], data, (len + 1) * sizeof(WCHAR)); + str->length += len; + return S_OK; +} + +/** Convert and append PSTR (char*) */ +static HRESULT WineWStrAppendA(WINEWSTR *str, UINT codepage, const char *data) +{ + HRESULT hr; + size_t len; + + if (!data[0]) + return S_OK; + + len = MultiByteToWideChar(codepage, 0, data, -1, NULL, 0); + hr = WineWStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + return hr; + + len = MultiByteToWideChar(codepage, 0, data, -1, &str->data[str->length], str->capacity + 1); + if (len <= 0) + return HRESULT_FROM_WIN32(GetLastError()); + + str->length += len - 1; + return S_OK; +} + +/** String format and append to the container */ +static HRESULT WINAPIV WineWStrAppendFmt(WINEWSTR *str, const WCHAR *fmt, ...) +{ + HRESULT hr; + int len; + va_list valist; + + va_start(valist, fmt); + len = vswprintf(NULL, -1, fmt, valist); + if (len < 0) { + hr = E_UNEXPECTED; + goto end; + } + + hr = WineWStrResize(str, (str->length + len) * 2); + if (hr != S_OK) + goto end; + + vswprintf(&str->data[str->length], -1, fmt, valist); + str->length += len; + +end: + va_end(valist); + return hr; +} + +#undef __WINE_PRINTF_ATTR + +#endif /* __WINE_WINE_STRING_H */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/910
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=123958 Your paranoid android. === w10pro64 (testbot log) === WineRunTask.pl:error: The previous 1 run(s) terminated abnormally === w8 (testbot log) === WineRunTask.pl:error: The previous 1 run(s) terminated abnormally === w7u_2qxl (testbot log) === WineRunTask.pl:error: The previous 1 run(s) terminated abnormally === w7u_2qxl (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w7u_adm (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w7u_el (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w8 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w8adm (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w864 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064v1507 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064v1809 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064_tsign (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64 (32 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w864 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064v1507 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064v1809 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064_2qxl (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064_adm (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w1064_tsign (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64_en_AE_u8 (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64_ar (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64_ja (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0 === w10pro64_zh_CN (64 bit report) === msvcrt: winestring.c:172: Test failed: @appendFmt winestring.c:173: Test failed: @strcmp, len=0 winestring.c:182: Test failed: @appendFmt winestring.c:183: Test failed: @lstrcmpW, len=0
I doubt we want to do it. Wine is generally reluctant to adding Wine specific public exports, there are only a few exceptions. Then, I think we absolutely don't want to replace each on stack string with a dynamically managed one, at least due to performance reasons. I guess if there are some problematic places (potentially) performing out of bounds access those should be fixed directly instead with proper size estimation and error checking.
On Wed Sep 21 21:55:31 2022 +0000, **** wrote:
Paul Gofman replied on the mailing list: ``` I doubt we want to do it. Wine is generally reluctant to adding Wine specific public exports, there are only a few exceptions. Then, I think we absolutely don't want to replace each on stack string with a dynamically managed one, at least due to performance reasons. I guess if there are some problematic places (potentially) performing out of bounds access those should be fixed directly instead with proper size estimation and error checking. ``` I agree with Paul, it's not something we want to add.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/910#note_9052
On Thu Sep 22 12:12:12 2022 +0000, Piotr Caban wrote:
I agree with Paul, it's not something we want to add. You are right that it is not necessary to replace all stack buffers. In many cases the bounds are handled properly. However, while fixing stack overflows in shell32/shlexec.c, I repeatedly used same malloc & strlen functions for manual length calculations, which is error-prone. Furthermore, the final string length can sometimes not be easily computed (e.g. `SHELL_ArgifyW`). That is why I came to the conclusion that moving basic operations (i.e. string concat or string formatting) to helper functions would be the safest and most convenient approach. In performance-critical cases the container should be preallocated with the expected final size (where applicable) to avoid realloc overhead.
--- On another note: It seems that there is a mismatch in (wine)gcc vs mingw compiler flags and/or behavior? Even `-Wall` in (wine)gcc did not show any of the warnings which are shown in the buildbot log. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/910#note_9083
On 9/22/22 14:16, Stefan Rentsch (@restet) wrote:
On Thu Sep 22 12:12:12 2022 +0000, Piotr Caban wrote:
I agree with Paul, it's not something we want to add. You are right that it is not necessary to replace all stack buffers. In many cases the bounds are handled properly. However, while fixing stack overflows in shell32/shlexec.c, I repeatedly used same malloc & strlen functions for manual length calculations, which is error-prone. Furthermore, the final string length can sometimes not be easily computed (e.g. `SHELL_ArgifyW`). That is why I came to the conclusion that moving basic operations (i.e. string concat or string formatting) to helper functions would be the safest and most convenient approach. In performance-critical cases the container should be preallocated with the expected final size (where applicable) to avoid realloc overhead. If there is a common pattern in shell32 maybe it worth considering some helper functions specific to shell32 module? Again, it is unlikely beneficial to have a generic custom string container implementation, but maybe moving a repeated check / copy pattern into a helper function may be fine if looks good.
On Thu Sep 22 20:13:56 2022 +0000, **** wrote:
Paul Gofman replied on the mailing list: ``` On 9/22/22 14:16, Stefan Rentsch (@restet) wrote:
On Thu Sep 22 12:12:12 2022 +0000, Piotr Caban wrote:
I agree with Paul, it's not something we want to add. You are right that it is not necessary to replace all stack buffers. In many cases the bounds are handled properly. However, while fixing stack overflows in shell32/shlexec.c, I repeatedly used same malloc & strlen functions for manual length calculations, which is error-prone. Furthermore, the final string length can sometimes not be easily computed (e.g. `SHELL_ArgifyW`). That is why I came to the conclusion that moving basic operations (i.e. string concat or string formatting) to helper functions would be the safest and most convenient approach. In performance-critical cases the container should be preallocated with the expected final size (where applicable) to avoid realloc overhead. If there is a common pattern in shell32 maybe it worth considering some helper functions specific to shell32 module? Again, it is unlikely beneficial to have a generic custom string container implementation, but maybe moving a repeated check / copy pattern into a helper function may be fine if looks good.
What exactly makes a generic implementation *not* beneficial in long-term? Currently there are least the following places that use dynamic string containers in private helper functions:
* atl/registrar.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/atl/registrar.c#L6... * jscript/json.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/jscript/json.c#L36... * mshtml/range.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/mshtml/range.c#L35... * ntdll/unix/env.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/ntdll/unix/env.c#L... (dynamic string, no container) * vbscript/vbregexp.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/vbscript/vbregexp.... * wineps.drv/type1.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/wineps.drv/type1.c... * winex11.drv/clipboard.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/winex11.drv/clipbo... (dynamic string, no container) Each implementation seems to be written from scratch, introducing code duplication. I could copy & paste a few functions for shell32 to ensure safe concat and bounds checks, which however again introduces more code to maintain, if that's the preferred practice. EDIT: I am open to suggestions. Please let me know if I can do something. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/910#note_9207
participants (5)
-
Marvin -
Paul Gofman -
Piotr Caban (＠piotr) -
Stefan Rentsch
-
Stefan Rentsch (＠restet)