Module: wine Branch: master Commit: 73d2f2285b0436612793ef29ebec3f3f60aa9b3f URL: https://gitlab.winehq.org/wine/wine/-/commit/73d2f2285b0436612793ef29ebec3f3...
Author: Alex Henrie alexhenrie24@gmail.com Date: Tue Jul 9 00:42:51 2024 -0600
kernelbase: Ensure null termination in RegGetValue[AW].
---
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..8471376bf96 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 < sizeof(WCHAR) || (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 (cbData < sizeof(WCHAR) || *((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 (!cbData || *((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);