[PATCH v5 0/6] MR5969: Draft: kernelbase: Ensure null termination in RegGetValue[AW].
I was hoping that the new tests would provide evidence that RtlQueryRegistryValues uses RegGetValue to ensure that multi-strings are double-null-terminated. Instead the tests suggest that my hypothesis was wrong because RegGetValue only ensures single null termination, not double null termination. We should fix RegGetValue anyway for the sake of applications that expect it to null-terminate. -- v5: kernelbase: Ensure null termination in RegGetValue[AW]. windowscodecs: Use RegQueryValueExW in ComponentInfo_GetStringValue. twinapi.appcore: Initialize size argument to RegGetValueW. shell32: Pass size in bytes to RegGetValueW. msi: Initialize size argument to GetRegValueW. mscoree: Pass size in bytes to RegGetValueW. https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/mscoree/corruntimehost.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/dlls/mscoree/corruntimehost.c b/dlls/mscoree/corruntimehost.c index f5a3831c90e..d9864b4dcd2 100644 --- a/dlls/mscoree/corruntimehost.c +++ b/dlls/mscoree/corruntimehost.c @@ -1738,8 +1738,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) WCHAR codebase[MAX_PATH + 8]; WCHAR classname[350], subkeyName[256]; WCHAR filename[MAX_PATH]; - - DWORD dwBufLen = 350; + DWORD dwBufLen; lstrcpyW(path, wszCLSIDSlash); StringFromGUID2(clsid, path + lstrlenW(wszCLSIDSlash), CHARS_IN_GUID); @@ -1758,7 +1757,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) HRESULT (WINAPI *pDllGetClassObject)(REFCLSID,REFIID,LPVOID*); IClassFactory *classfactory; - dwBufLen = ARRAY_SIZE( filename ); + dwBufLen = sizeof( filename ); res = RegGetValueW( subkey, NULL, NULL, RRF_RT_REG_SZ, NULL, filename, &dwBufLen ); RegCloseKey( subkey ); @@ -1796,6 +1795,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) goto cleanup; } + dwBufLen = sizeof( classname ); res = RegGetValueW( key, NULL, L"Class", RRF_RT_REG_SZ, NULL, classname, &dwBufLen); if(res != ERROR_SUCCESS) { @@ -1806,7 +1806,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) TRACE("classname (%s)\n", debugstr_w(classname)); - dwBufLen = MAX_PATH + 8; + dwBufLen = sizeof( codebase ); res = RegGetValueW( key, NULL, L"CodeBase", RRF_RT_REG_SZ, NULL, codebase, &dwBufLen); if(res == ERROR_SUCCESS) { @@ -1841,7 +1841,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) res = RegOpenKeyExW(key, subkeyName, 0, KEY_READ, &subkey); if (res != ERROR_SUCCESS) goto cleanup; - dwBufLen = MAX_PATH + 8; + dwBufLen = sizeof( assemblyname ); res = RegGetValueW(subkey, NULL, L"Assembly", RRF_RT_REG_SZ, NULL, assemblyname, &dwBufLen); RegCloseKey(subkey); if (res != ERROR_SUCCESS) @@ -1849,7 +1849,7 @@ HRESULT create_monodata(REFCLSID clsid, LPVOID *ppObj) } else { - dwBufLen = MAX_PATH + 8; + dwBufLen = sizeof( assemblyname ); res = RegGetValueW(key, NULL, L"Assembly", RRF_RT_REG_SZ, NULL, assemblyname, &dwBufLen); if (res != ERROR_SUCCESS) goto cleanup; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/msi/registry.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/msi/registry.c b/dlls/msi/registry.c index 149f9cf9945..ad220eaa756 100644 --- a/dlls/msi/registry.c +++ b/dlls/msi/registry.c @@ -1846,6 +1846,7 @@ static UINT check_product_patches(const WCHAR *prodcode, const WCHAR *usersid, M RegOpenKeyExW(localprod, L"Patches", 0, KEY_READ, &localpatch) == ERROR_SUCCESS && RegOpenKeyExW(localpatch, ptr, 0, KEY_READ, &patchkey) == ERROR_SUCCESS) { + size = sizeof(state); res = RegGetValueW(patchkey, NULL, L"State", RRF_RT_REG_DWORD, &type, &state, &size); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/shell32/shfldr_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/shell32/shfldr_fs.c b/dlls/shell32/shfldr_fs.c index 928cadfc77c..bc3cb000f0e 100644 --- a/dlls/shell32/shfldr_fs.c +++ b/dlls/shell32/shfldr_fs.c @@ -588,7 +588,7 @@ static HRESULT SHELL32_CreateExtensionUIObject(IShellFolder2 *iface, IPersistFile *persist_file; char extensionA[20]; WCHAR extensionW[20], buf[MAX_PATH]; - DWORD size = MAX_PATH; + DWORD size = sizeof(buf); STRRET path; WCHAR *file; GUID guid; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/twinapi.appcore/analytics_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/twinapi.appcore/analytics_info.c b/dlls/twinapi.appcore/analytics_info.c index abbdb621e1d..48538778b7d 100644 --- a/dlls/twinapi.appcore/analytics_info.c +++ b/dlls/twinapi.appcore/analytics_info.c @@ -97,7 +97,7 @@ static HRESULT WINAPI analytics_version_info_get_DeviceFamily( IAnalyticsVersion static HRESULT WINAPI analytics_version_info_get_DeviceFamilyVersion( IAnalyticsVersionInfo *iface, HSTRING *value ) { - DWORD revision, size; + DWORD revision, size = sizeof(DWORD); WCHAR buffer[32]; UINT64 version; UINT len; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/windowscodecs/info.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/windowscodecs/info.c b/dlls/windowscodecs/info.c index f42fc8932c9..67704365c06 100644 --- a/dlls/windowscodecs/info.c +++ b/dlls/windowscodecs/info.c @@ -50,8 +50,7 @@ static HRESULT ComponentInfo_GetStringValue(HKEY classkey, LPCWSTR value, if (!actual_size) return E_INVALIDARG; - ret = RegGetValueW(classkey, NULL, value, RRF_RT_REG_SZ|RRF_NOEXPAND, NULL, - buffer, &cbdata); + ret = RegQueryValueExW(classkey, value, 0, NULL, (void *)buffer, &cbdata); if (ret == ERROR_FILE_NOT_FOUND) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/advapi32/tests/registry.c | 140 ++++++++++++++++++++++++++++++++- dlls/kernelbase/registry.c | 26 ++++++ 2 files changed, 163 insertions(+), 3 deletions(-) diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c index 2afebb3365b..0f77b2fa066 100644 --- a/dlls/advapi32/tests/registry.c +++ b/dlls/advapi32/tests/registry.c @@ -911,12 +911,16 @@ static void test_get_value(void) /* Query REG_SZ using RRF_RT_REG_SZ on a zero-byte value (ok) */ strcpy(buf, sTestpath1); type = 0xdeadbeef; - size = sizeof(buf); + size = 0; + ret = pRegGetValueA(hkey_main, NULL, "TP1_ZB_SZ", RRF_RT_REG_SZ, &type, NULL, &size); + ok(ret == ERROR_SUCCESS, "ret=%ld\n", ret); + ok(size == 1, "size=%ld\n", size); + ok(type == REG_SZ, "type=%ld\n", type); ret = pRegGetValueA(hkey_main, NULL, "TP1_ZB_SZ", RRF_RT_REG_SZ, &type, buf, &size); ok(ret == ERROR_SUCCESS, "ret=%ld\n", ret); - todo_wine ok(size == 1, "size=%ld\n", size); + ok(size == 1, "size=%ld\n", size); ok(type == REG_SZ, "type=%ld\n", type); - todo_wine ok(!strcmp(buf, ""), "Expected \"\", got \"%s\"\n", buf); + ok(!strcmp(buf, ""), "Expected \"\", got \"%s\"\n", buf); /* Query REG_SZ using RRF_RT_REG_SZ|RRF_NOEXPAND (ok) */ buf[0] = 0; type = 0xdeadbeef; size = sizeof(buf); @@ -2210,6 +2214,135 @@ static void test_string_termination(void) debugstr_an((char*)buffer, outsize), outsize, string); ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + /* RegGetValueA always adds the trailing '\0' */ + if (pRegGetValueA) + { + outsize = insize; + ret = pRegGetValueA(subkey, NULL, "stringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); + ok(ret == ERROR_MORE_DATA, "RegGetValueA returned: %ld\n", ret); + ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + memset(buffer, 0xbd, sizeof(buffer)); + ret = pRegGetValueA(subkey, NULL, "stringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegGetValueA returned: %ld\n", ret); + ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + ok(memcmp(buffer, string, insize) == 0, "bad string: %s/%lu != %s\n", + debugstr_an((char*)buffer, insize), insize, string); + ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + } + + RegDeleteKeyA(subkey, ""); + RegCloseKey(subkey); +} + +static void test_multistring_termination(void) +{ + HKEY subkey; + LSTATUS ret; + static const char multistring[] = "Aa\0Bb\0Cc\0"; + char name[sizeof("multistringtest")]; + BYTE buffer[sizeof(multistring)]; + DWORD insize, outsize, nsize; + + ret = RegCreateKeyA(hkey_main, "multistring_termination", &subkey); + ok(ret == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %ld\n", ret); + + /* Off-by-one RegSetValueExA -> only one trailing '\0' */ + insize = sizeof(multistring) - 1; + ret = RegSetValueExA(subkey, "multistringtest", 0, REG_SZ, (BYTE*)multistring, insize); + ok(ret == ERROR_SUCCESS, "RegSetValueExA failed: %ld\n", ret); + outsize = 0; + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, NULL, &outsize); + ok(ret == ERROR_SUCCESS, "RegQueryValueExA failed: %ld\n", ret); + ok(outsize == insize, "wrong size %lu != %lu\n", outsize, insize); + + /* Off-by-two RegSetValueExA -> adds a trailing '\0'! */ + insize = sizeof(multistring) - 2; + ret = RegSetValueExA(subkey, "multistringtest", 0, REG_SZ, (BYTE*)multistring, insize); + ok(ret == ERROR_SUCCESS, "RegSetValueExA failed: %ld\n", ret); + outsize = insize; + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, buffer, &outsize); + ok(ret == ERROR_MORE_DATA, "RegQueryValueExA returned: %ld\n", ret); + + /* Off-by-three RegSetValueExA -> no trailing '\0' */ + insize = sizeof(multistring) - 3; + ret = RegSetValueExA(subkey, "multistringtest", 0, REG_SZ, (BYTE*)multistring, insize); + ok(ret == ERROR_SUCCESS, "RegSetValueExA failed: %ld\n", ret); + outsize = 0; + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, NULL, &outsize); + ok(ret == ERROR_SUCCESS, "RegQueryValueExA failed: %ld\n", ret); + ok(outsize == insize, "wrong size %lu != %lu\n", outsize, insize); + + /* RegQueryValueExA may return a multistring with no trailing '\0' */ + outsize = insize; + memset(buffer, 0xbd, sizeof(buffer)); + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegQueryValueExA failed: %ld\n", ret); + ok(outsize == insize, "wrong size: %lu != %lu\n", outsize, insize); + ok(memcmp(buffer, multistring, outsize) == 0, "bad multistring: %s/%lu != %s\n", + debugstr_an((char*)buffer, outsize), outsize, multistring); + ok(buffer[insize] == 0xbd, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + + /* RegQueryValueExA adds one trailing '\0' if there is room */ + outsize = insize + 1; + memset(buffer, 0xbd, sizeof(buffer)); + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegQueryValueExA failed: %ld\n", ret); + ok(outsize == insize, "wrong size: %lu != %lu\n", outsize, insize); + ok(memcmp(buffer, multistring, outsize) == 0, "bad multistring: %s/%lu != %s\n", + debugstr_an((char*)buffer, outsize), outsize, multistring); + ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + + /* RegQueryValueExA doesn't add a second trailing '\0' even if there is room */ + outsize = insize + 2; + memset(buffer, 0xbd, sizeof(buffer)); + ret = RegQueryValueExA(subkey, "multistringtest", NULL, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegQueryValueExA failed: %ld\n", ret); + ok(outsize == insize, "wrong size: %lu != %lu\n", outsize, insize); + ok(memcmp(buffer, multistring, outsize) == 0, "bad multistring: %s/%lu != %s\n", + debugstr_an((char*)buffer, outsize), outsize, multistring); + ok(buffer[insize + 1] == 0xbd, "buffer overflow at %lu %02x\n", insize, buffer[insize + 1]); + + /* RegEnumValueA may return a multistring with no trailing '\0' */ + outsize = insize; + memset(buffer, 0xbd, sizeof(buffer)); + nsize = sizeof(name); + ret = RegEnumValueA(subkey, 0, name, &nsize, NULL, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegEnumValueA failed: %ld\n", ret); + ok(strcmp(name, "multistringtest") == 0, "wrong name: %s\n", name); + ok(outsize == insize, "wrong size: %lu != %lu\n", outsize, insize); + ok(memcmp(buffer, multistring, outsize) == 0, "bad multistring: %s/%lu != %s\n", + debugstr_an((char*)buffer, outsize), outsize, multistring); + ok(buffer[insize] == 0xbd, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + + /* RegEnumValueA adds one trailing '\0' even if there's room for two */ + outsize = insize + 2; + memset(buffer, 0xbd, sizeof(buffer)); + nsize = sizeof(name); + ret = RegEnumValueA(subkey, 0, name, &nsize, NULL, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegEnumValueA failed: %ld\n", ret); + ok(strcmp(name, "multistringtest") == 0, "wrong name: %s\n", name); + ok(outsize == insize, "wrong size: %lu != %lu\n", outsize, insize); + ok(memcmp(buffer, multistring, outsize) == 0, "bad multistring: %s/%lu != %s\n", + debugstr_an((char*)buffer, outsize), outsize, multistring); + ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + ok(buffer[insize + 1] == 0xbd, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + + /* RegGetValueA always adds one trailing '\0' even if there's room for two */ + if (pRegGetValueA) + { + outsize = insize; + ret = pRegGetValueA(subkey, NULL, "multistringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); + ok(ret == ERROR_MORE_DATA, "RegGetValueA returned: %ld\n", ret); + ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + outsize = insize + 2; + memset(buffer, 0xbd, sizeof(buffer)); + ret = pRegGetValueA(subkey, NULL, "multistringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); + ok(ret == ERROR_SUCCESS, "RegGetValueA returned: %ld\n", ret); + ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize + 1]); + ok(buffer[insize + 1] == 0xbd, "buffer overflow at %lu %02x\n", insize, buffer[insize + 1]); + } + RegDeleteKeyA(subkey, ""); RegCloseKey(subkey); } @@ -5007,6 +5140,7 @@ START_TEST(registry) test_reg_query_value(); test_reg_query_info(); test_string_termination(); + test_multistring_termination(); test_symlinks(); test_redirection(); test_classesroot(); diff --git a/dlls/kernelbase/registry.c b/dlls/kernelbase/registry.c index ee57a4acdd1..0c5e8495e0c 100644 --- a/dlls/kernelbase/registry.c +++ b/dlls/kernelbase/registry.c @@ -2008,6 +2008,19 @@ LSTATUS WINAPI RegGetValueW( HKEY hKey, LPCWSTR pszSubKey, LPCWSTR pszValue, if (pszSubKey && pszSubKey[0]) RegCloseKey(hKey); + /* Ensure null termination */ + if (is_string(dwType) && (!cbData || (pvData && *((WCHAR *)pvData + cbData / sizeof(WCHAR) - 1)))) + { + if (pvData) + { + if (cbData + sizeof(WCHAR) <= *pcbData) + *((WCHAR *)pvData + cbData / sizeof(WCHAR)) = 0; + else + ret = ERROR_MORE_DATA; + } + cbData += sizeof(WCHAR); + } + apply_restrictions(dwFlags, dwType, cbData, &ret); if (pvData && ret != ERROR_SUCCESS && (dwFlags & RRF_ZEROONFAILURE)) @@ -2113,6 +2126,19 @@ LSTATUS WINAPI RegGetValueA( HKEY hKey, LPCSTR pszSubKey, LPCSTR pszValue, if (pszSubKey && pszSubKey[0]) RegCloseKey(hKey); + /* Ensure null termination */ + if (is_string(dwType) && (!cbData || (pvData && *((char *)pvData + cbData - 1)))) + { + if (pvData) + { + if (cbData < *pcbData) + *((char *)pvData + cbData) = 0; + else + ret = ERROR_MORE_DATA; + } + cbData++; + } + apply_restrictions(dwFlags, dwType, cbData, &ret); if (pvData && ret != ERROR_SUCCESS && (dwFlags & RRF_ZEROONFAILURE)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5969
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=146784 Your paranoid android. === debian11 (32 bit report) === shell32: shellpath.c:2558: Test failed: failed to redirect known folder: 0x80070002, errorMsg: (null) shellpath.c:2563: Test failed: invalid known folder path retrieved: "L"C:\\users\\winetest\\Temp\\Example2"" when "L"C:\\users\\winetest\\Temp\\Example"" was expected shellpath.c:2569: Test failed: invalid known folder path retrieved: "L"C:\\users\\winetest\\Temp\\Example2\\SubFolder"" when "L"C:\\users\\winetest\\Temp\\Example\\SubFolder"" was expected === debian11b (64 bit WoW report) === dinput: joystick8.c:5408: Test failed: GetRawInputDeviceList returned 0 user32: input.c:5898: Test failed: got pos (49,51) input.c:2206: Test failed: got 0 messages input.c:2216: Test failed: got 0 messages input.c:1933: Test failed: expected non-zero input.c:1939: Test failed: expected -1, got 0 input.c:1940: Test failed: expected 122, got -559038737 input.c:1941: Test failed: expected non-zero input.c:1945: Test failed: expected non-zero input.c:2080: Test failed: expected non-zero
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Marvin