[PATCH v2 0/2] MR5969: 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. -- v2: kernelbase: Ensure null termination in RegGetValue[AW]. windowscodecs: Use RegQueryValueExW in ComponentInfo_GetStringValue. 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 | 20 +++++ 2 files changed, 157 insertions(+), 3 deletions(-) diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c index 2afebb3365b..baeeaa019d0 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, buf, &size); + ok(ret == ERROR_MORE_DATA, "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..6a434f7e983 100644 --- a/dlls/kernelbase/registry.c +++ b/dlls/kernelbase/registry.c @@ -2008,6 +2008,16 @@ 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 && 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 +2123,16 @@ 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 && 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=146743 Your paranoid android. === debian11b (64 bit WoW report) === dinput: joystick8.c:5408: Test failed: GetRawInputDeviceList returned 0 user32: 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