[PATCH 0/1] MR5969: kernelbase: Ensure null termination in RegGetValue[AW].

[Alex Henrie (＠alexhenrie)]

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.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5969

[PATCH 1/1] kernelbase: Ensure null termination in RegGetValue[AW].

[Alex Henrie]

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

[PATCH 1/1] kernelbase: Ensure null termination in RegGetValue[AW].

[Marvin]

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=146740

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

windowscodecs:
info.c:329: Test failed: got length of 0x12, expected 0x11
info.c:379: Test failed: GetSpecVersion failed, hr=88982f8c
info.c:380: Test failed: invalid length 0x1