[PATCH 0/2] MR5895: ntdll: Don't write partial strings with RTL_QUERY_REGISTRY_DIRECT.
The new tests revealed only one case where too-small buffers were not already being handled correctly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5895
From: Alex Henrie <alexhenrie24(a)gmail.com> The new tests revealed only one case where too-small buffers were not already being handled correctly. --- dlls/ntdll/reg.c | 3 ++- dlls/ntdll/tests/reg.c | 54 ++++++++++++++++++++++++++++++------------ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/dlls/ntdll/reg.c b/dlls/ntdll/reg.c index 137bd63c0a8..a70f95a6807 100644 --- a/dlls/ntdll/reg.c +++ b/dlls/ntdll/reg.c @@ -327,7 +327,8 @@ static NTSTATUS RTL_ReportRegistryValue(PKEY_VALUE_FULL_INFORMATION pInfo, str->Buffer = RtlAllocateHeap(GetProcessHeap(), 0, len); str->MaximumLength = len; } - len = min(len, str->MaximumLength); + else if (str->MaximumLength < len) + return STATUS_BUFFER_TOO_SMALL; len -= sizeof(WCHAR); memcpy(str->Buffer, ((CHAR*)pInfo) + pInfo->DataOffset, len); str->Buffer[len / sizeof(WCHAR)] = 0; diff --git a/dlls/ntdll/tests/reg.c b/dlls/ntdll/tests/reg.c index ca30eabcb9c..2bd77290ab9 100644 --- a/dlls/ntdll/tests/reg.c +++ b/dlls/ntdll/tests/reg.c @@ -2624,7 +2624,8 @@ static NTSTATUS WINAPI query_routine(const WCHAR *value_name, ULONG value_type, return STATUS_SUCCESS; } -static UNICODE_STRING query_reg_values_direct_str; +static WCHAR query_reg_values_direct_str_buf[32]; +static UNICODE_STRING query_reg_values_direct_str = {0, 0, query_reg_values_direct_str_buf}; static ULONG query_reg_values_direct_int; @@ -2689,16 +2690,25 @@ static struct query_reg_values_test query_reg_values_tests[] = }, { {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"WindowsDrive", &query_reg_values_direct_str }}, - STATUS_SUCCESS, 0, 0, REG_SZ, L"C:" + STATUS_SUCCESS, 0, 0, REG_NONE, L"C:" + }, + { + {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"WindowsDrive", &query_reg_values_direct_str }}, + STATUS_SUCCESS, 0, 0, REG_NONE, L"\x2323", 0, 2 * sizeof(WCHAR) }, { {{ NULL, RTL_QUERY_REGISTRY_DIRECT | RTL_QUERY_REGISTRY_NOEXPAND, (WCHAR*)L"WindowsDrive", &query_reg_values_direct_str }}, - STATUS_SUCCESS, 0, 0, REG_SZ, L"%SYSTEMDRIVE%" + STATUS_SUCCESS, 0, 0, REG_NONE, L"%SYSTEMDRIVE%" + }, + { + {{ NULL, RTL_QUERY_REGISTRY_DIRECT | RTL_QUERY_REGISTRY_NOEXPAND, (WCHAR*)L"WindowsDrive", + &query_reg_values_direct_str }}, + STATUS_SUCCESS, 0, 0, REG_NONE, L"\x2323", 0, 2 * sizeof(WCHAR) }, { {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"MeaningOfLife32", &query_reg_values_direct_int }}, - STATUS_SUCCESS, 0, 0, REG_DWORD, (WCHAR*)42 + STATUS_SUCCESS, 0, 0, REG_NONE, (WCHAR*)42 }, { {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"MeaningOfLife64", &query_reg_values_direct_sized }}, @@ -2724,7 +2734,12 @@ static struct query_reg_values_test query_reg_values_tests[] = { {{ NULL, RTL_QUERY_REGISTRY_DIRECT | RTL_QUERY_REGISTRY_NOEXPAND, (WCHAR*)L"CapitalsOfEurope", &query_reg_values_direct_str }}, - STATUS_SUCCESS, 0, 0, REG_SZ, L"Brussels\0Paris\0%PATH%\0", sizeof(L"Brussels\0Paris\0%PATH%\0") + STATUS_SUCCESS, 0, 0, REG_NONE, L"Brussels\0Paris\0%PATH%\0", sizeof(L"Brussels\0Paris\0%PATH%\0") + }, + { + {{ NULL, RTL_QUERY_REGISTRY_DIRECT | RTL_QUERY_REGISTRY_NOEXPAND, (WCHAR*)L"CapitalsOfEurope", + &query_reg_values_direct_str }}, + STATUS_SUCCESS, 0, 0, REG_NONE, L"\x2323", 0, 2 * sizeof(WCHAR) }, /* DIRECT with a null buffer crashes on Windows */ /* { @@ -2788,15 +2803,25 @@ static struct query_reg_values_test query_reg_values_tests[] = &query_reg_values_direct_str, REG_SZ, (WCHAR*)L"%SYSTEMDRIVE%" }}, STATUS_SUCCESS, 0, EXPECT_DEFAULT_DATA }, + { + {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"I don't exist", + &query_reg_values_direct_str, REG_SZ, (WCHAR*)L"%SYSTEMDRIVE%" }}, + STATUS_SUCCESS, 0, 0, REG_NONE, L"\x2323", 0, 2 * sizeof(WCHAR) + }, { {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"I don't exist", &query_reg_values_direct_str, REG_EXPAND_SZ, (WCHAR*)L"%SYSTEMDRIVE%" }}, - STATUS_SUCCESS, 0, WINE_TODO_SIZE | WINE_TODO_DATA, REG_EXPAND_SZ, L"C:" + STATUS_SUCCESS, 0, WINE_TODO_SIZE | WINE_TODO_DATA, REG_NONE, L"C:" + }, + { + {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"I don't exist", + &query_reg_values_direct_str, REG_EXPAND_SZ, (WCHAR*)L"%SYSTEMDRIVE%" }}, + STATUS_SUCCESS, 0, WINE_TODO_SIZE, REG_NONE, L"\x2323", 0, 2 * sizeof(WCHAR) }, /* DIRECT with a multi-string default value crashes on Windows */ /* { {{ NULL, RTL_QUERY_REGISTRY_DIRECT, (WCHAR*)L"I don't exist", - &query_reg_values_direct_str, REG_MULTI_SZ, (WCHAR*)L"A\0B\0C\0", sizeof(L"A\0B\0C\0") }}, + &query_reg_values_direct_str, REG_NONE, (WCHAR*)L"A\0B\0C\0", sizeof(L"A\0B\0C\0") }}, STATUS_SUCCESS, EXPECT_DEFAULT_DATA }, */ /* The default value is not used if it is not valid */ @@ -2871,10 +2896,6 @@ static void test_RtlQueryRegistryValues(void) L"Yellow", sizeof(L"Yellow")); ok(status == ERROR_SUCCESS, "Failed to create registry value Color: %lu\n", status); - query_reg_values_direct_str.MaximumLength = 32 * sizeof(WCHAR); - query_reg_values_direct_str.Buffer = pRtlAllocateHeap(GetProcessHeap(), 0, - query_reg_values_direct_str.MaximumLength); - for (i = 0; i < ARRAY_SIZE(query_reg_values_tests); i++) { struct query_reg_values_test *test = &query_reg_values_tests[i]; @@ -2893,9 +2914,14 @@ static void test_RtlQueryRegistryValues(void) query_routine_calls = 0; - query_reg_values_direct_str.Length = query_reg_values_direct_str.MaximumLength - sizeof(WCHAR); + query_reg_values_direct_str.MaximumLength = test->size_limit ? test->size_limit + : sizeof(query_reg_values_direct_str_buf); + if (query_reg_values_direct_str.MaximumLength >= sizeof(WCHAR)) + query_reg_values_direct_str.Length = query_reg_values_direct_str.MaximumLength - sizeof(WCHAR); + else + query_reg_values_direct_str.Length = 0; memset(query_reg_values_direct_str.Buffer, 0x23, query_reg_values_direct_str.Length); - query_reg_values_direct_str.Buffer[query_reg_values_direct_str.Length] = 0; + query_reg_values_direct_str.Buffer[query_reg_values_direct_str.Length / sizeof(WCHAR)] = 0; query_reg_values_direct_int = 1; @@ -2992,8 +3018,6 @@ static void test_RtlQueryRegistryValues(void) status = RegDeleteKeyValueW(HKEY_CURRENT_USER, L"WineTest", L"WindowsDrive"); ok(status == ERROR_FILE_NOT_FOUND, "Registry value WindowsDrive should have been deleted already\n"); - - pRtlFreeHeap(GetProcessHeap(), 0, query_reg_values_direct_str.Buffer); } START_TEST(reg) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5895
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/ntdll/reg.c | 3 +-- dlls/ntdll/tests/reg.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/ntdll/reg.c b/dlls/ntdll/reg.c index a70f95a6807..d39c1a34bb7 100644 --- a/dlls/ntdll/reg.c +++ b/dlls/ntdll/reg.c @@ -376,8 +376,7 @@ static NTSTATUS RTL_ReportRegistryValue(PKEY_VALUE_FULL_INFORMATION pInfo, dst.MaximumLength = res; dst.Buffer = RtlAllocateHeap(GetProcessHeap(), 0, res * sizeof(WCHAR)); RtlExpandEnvironmentStrings_U(pEnvironment, &src, &dst, &res); - status = pQuery->QueryRoutine(pQuery->Name, pInfo->Type, dst.Buffer, - dst.Length, pContext, pQuery->EntryContext); + status = pQuery->QueryRoutine(pQuery->Name, REG_SZ, dst.Buffer, res, pContext, pQuery->EntryContext); RtlFreeHeap(GetProcessHeap(), 0, dst.Buffer); } else /* REG_MULTI_SZ */ diff --git a/dlls/ntdll/tests/reg.c b/dlls/ntdll/tests/reg.c index 2bd77290ab9..e1ca45b9320 100644 --- a/dlls/ntdll/tests/reg.c +++ b/dlls/ntdll/tests/reg.c @@ -2863,7 +2863,7 @@ static struct query_reg_values_test query_reg_values_tests[] = /* DELETE deletes the value after reading it */ { {{ query_routine, RTL_QUERY_REGISTRY_DELETE, (WCHAR*)L"WindowsDrive" }}, - STATUS_SUCCESS, 1, WINE_TODO_TYPE | WINE_TODO_SIZE, REG_SZ, L"C:" + STATUS_SUCCESS, 1, 0, REG_SZ, L"C:" }, { {{ query_routine, 0, (WCHAR*)L"I don't exist", NULL, REG_SZ, (WCHAR*)L"Some default" }}, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5895
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=146467 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000002E800DA, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
participants (3)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Marvin