[PATCH v7 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. -- v7: kernelbase: Ensure null termination in RegGetValue[AW]. 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 | 20 ++++----- dlls/kernelbase/registry.c | 76 +++++++++++++++++++++------------- 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/dlls/advapi32/tests/registry.c b/dlls/advapi32/tests/registry.c index 17085cb6b1c..0f77b2fa066 100644 --- a/dlls/advapi32/tests/registry.c +++ b/dlls/advapi32/tests/registry.c @@ -914,13 +914,13 @@ static void test_get_value(void) 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); - todo_wine ok(size == 1, "size=%ld\n", size); + 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); @@ -2219,15 +2219,15 @@ static void test_string_termination(void) { outsize = insize; ret = pRegGetValueA(subkey, NULL, "stringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); - todo_wine ok(ret == ERROR_MORE_DATA, "RegGetValueA returned: %ld\n", ret); - todo_wine ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + 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); - todo_wine ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + 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); - todo_wine ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); + ok(buffer[insize] == 0, "buffer overflow at %lu %02x\n", insize, buffer[insize]); } RegDeleteKeyA(subkey, ""); @@ -2332,13 +2332,13 @@ static void test_multistring_termination(void) { outsize = insize; ret = pRegGetValueA(subkey, NULL, "multistringtest", RRF_RT_REG_SZ, NULL, buffer, &outsize); - todo_wine ok(ret == ERROR_MORE_DATA, "RegGetValueA returned: %ld\n", ret); - todo_wine ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + 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); - todo_wine ok(outsize == insize + 1, "wrong size: %lu != %lu\n", outsize, insize + 1); + 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]); } diff --git a/dlls/kernelbase/registry.c b/dlls/kernelbase/registry.c index ee57a4acdd1..8cfaf448e3e 100644 --- a/dlls/kernelbase/registry.c +++ b/dlls/kernelbase/registry.c @@ -1955,16 +1955,18 @@ LSTATUS WINAPI RegGetValueW( HKEY hKey, LPCWSTR pszSubKey, LPCWSTR pszValue, ret = RegQueryValueExW(hKey, pszValue, NULL, &dwType, pvData, &cbData); - /* If we are going to expand we need to read in the whole the value even - * if the passed buffer was too small as the expanded string might be - * smaller than the unexpanded one and could fit into cbData bytes. */ - if ((ret == ERROR_SUCCESS || ret == ERROR_MORE_DATA) && - dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) + /* If the value is a string, we need to read in the whole value to be able + * to know exactly how many bytes are needed after expanding the string and + * ensuring that it is null-terminated. */ + if (is_string(dwType) && + (ret == ERROR_MORE_DATA || + (ret == ERROR_SUCCESS && dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) || + (ret == ERROR_SUCCESS && (!cbData || (pvData && *((WCHAR *)pvData + cbData / sizeof(WCHAR) - 1)))))) { do { heap_free(pvBuf); - pvBuf = heap_alloc(cbData); + pvBuf = heap_alloc(cbData + sizeof(WCHAR)); if (!pvBuf) { ret = ERROR_NOT_ENOUGH_MEMORY; @@ -1981,25 +1983,33 @@ LSTATUS WINAPI RegGetValueW( HKEY hKey, LPCWSTR pszSubKey, LPCWSTR pszValue, * overlapping buffers. */ CopyMemory(pvBuf, pvData, cbData); } - - /* Both the type or the value itself could have been modified in - * between so we have to keep retrying until the buffer is large - * enough or we no longer have to expand the value. */ - } while (dwType == REG_EXPAND_SZ && ret == ERROR_MORE_DATA); + } while (ret == ERROR_MORE_DATA); if (ret == ERROR_SUCCESS) { + /* Ensure null termination */ + if (*((WCHAR *)pvBuf + cbData / sizeof(WCHAR) - 1)) + { + *((WCHAR *)pvBuf + cbData / sizeof(WCHAR)) = 0; + cbData += sizeof(WCHAR); + } + /* Recheck dwType in case it changed since the first call */ - if (dwType == REG_EXPAND_SZ) + if (dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) { cbData = ExpandEnvironmentStringsW(pvBuf, pvData, pcbData ? *pcbData : 0) * sizeof(WCHAR); dwType = REG_SZ; - if(pvData && pcbData && cbData > *pcbData) + if (pvData && cbData > *pcbData) ret = ERROR_MORE_DATA; } else if (pvData) - CopyMemory(pvData, pvBuf, *pcbData); + { + if (cbData > *pcbData) + ret = ERROR_MORE_DATA; + else + CopyMemory(pvData, pvBuf, cbData); + } } heap_free(pvBuf); @@ -2060,16 +2070,18 @@ LSTATUS WINAPI RegGetValueA( HKEY hKey, LPCSTR pszSubKey, LPCSTR pszValue, ret = RegQueryValueExA(hKey, pszValue, NULL, &dwType, pvData, &cbData); - /* If we are going to expand we need to read in the whole the value even - * if the passed buffer was too small as the expanded string might be - * smaller than the unexpanded one and could fit into cbData bytes. */ - if ((ret == ERROR_SUCCESS || ret == ERROR_MORE_DATA) && - dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) + /* If the value is a string, we need to read in the whole value to be able + * to know exactly how many bytes are needed after expanding the string and + * ensuring that it is null-terminated. */ + if (is_string(dwType) && + (ret == ERROR_MORE_DATA || + (ret == ERROR_SUCCESS && dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) || + (ret == ERROR_SUCCESS && (!cbData || (pvData && *((char *)pvData + cbData - 1)))))) { do { heap_free(pvBuf); - pvBuf = heap_alloc(cbData); + pvBuf = heap_alloc(cbData + 1); if (!pvBuf) { ret = ERROR_NOT_ENOUGH_MEMORY; @@ -2086,25 +2098,33 @@ LSTATUS WINAPI RegGetValueA( HKEY hKey, LPCSTR pszSubKey, LPCSTR pszValue, * overlapping buffers. */ CopyMemory(pvBuf, pvData, cbData); } - - /* Both the type or the value itself could have been modified in - * between so we have to keep retrying until the buffer is large - * enough or we no longer have to expand the value. */ - } while (dwType == REG_EXPAND_SZ && ret == ERROR_MORE_DATA); + } while (ret == ERROR_MORE_DATA); if (ret == ERROR_SUCCESS) { + /* Ensure null termination */ + if (*((char *)pvBuf + cbData - 1)) + { + *((char *)pvBuf + cbData) = 0; + cbData++; + } + /* Recheck dwType in case it changed since the first call */ - if (dwType == REG_EXPAND_SZ) + if (dwType == REG_EXPAND_SZ && !(dwFlags & RRF_NOEXPAND)) { cbData = ExpandEnvironmentStringsA(pvBuf, pvData, pcbData ? *pcbData : 0); dwType = REG_SZ; - if(pvData && pcbData && cbData > *pcbData) + if (pvData && cbData > *pcbData) ret = ERROR_MORE_DATA; } else if (pvData) - CopyMemory(pvData, pvBuf, *pcbData); + { + if (cbData > *pcbData) + ret = ERROR_MORE_DATA; + else + CopyMemory(pvData, pvBuf, cbData); + } } heap_free(pvBuf); -- 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=146898 Your paranoid android. === debian11b (64 bit WoW report) === ddraw: ddraw1.c:3645: Test failed: Expected (0,0)-(640,480), got (-32000,-32000)-(-31840,-31969).
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Marvin