[PATCH v2 0/2] MR2618: ntdll: Increment offset by len in build_clr_surrogate_section.
Instead of multiplying it by 2. I don't know how this ever worked. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54808 -- v2: ntdll: Increment offset by len in build_clr_surrogate_section. kernel32/tests: Check that lpSectionBase != NULL before dereferencing. https://gitlab.winehq.org/wine/wine/-/merge_requests/2618
From: Alex Henrie <alexhenrie24(a)gmail.com> If lpSectionBase is null then the tests will still crash, but at least we'll know why. --- dlls/kernel32/tests/actctx.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index da351e70466..3d547e01b19 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -1454,8 +1454,8 @@ static void test_find_activatable_class(HANDLE handle, const WCHAR *classid, enu ok_(__FILE__, line)(data.lpData != NULL, "got lpData %p\n", data.lpData); header = (struct strsection_header *)data.lpSectionBase; - ok_(__FILE__, line)(header->magic == 0x64487353, "got wrong magic 0x%08lx\n", header->magic); ok_(__FILE__, line)(data.lpSectionBase != NULL, "got lpSectionBase %p\n", data.lpSectionBase); + ok_(__FILE__, line)(header->magic == 0x64487353, "got wrong magic 0x%08lx\n", header->magic); ok_(__FILE__, line)(data.ulSectionTotalLength > 0, "got ulSectionTotalLength %lu\n", data.ulSectionTotalLength); ok_(__FILE__, line)(data.lpSectionGlobalData == (BYTE *)header + header->global_offset, "got lpSectionGlobalData %p\n", data.lpSectionGlobalData); @@ -1706,10 +1706,10 @@ static void test_find_com_redirection(HANDLE handle, const GUID *clsid, const GU } header = (struct guidsection_header*)data.lpSectionBase; + ok_(__FILE__, line)(data.lpSectionBase != NULL, "data.lpSectionBase == NULL\n"); ok_(__FILE__, line)(data.lpSectionGlobalData == ((BYTE*)header + header->names_offset), "data.lpSectionGlobalData == NULL\n"); ok_(__FILE__, line)(data.ulSectionGlobalDataLength == header->names_len, "data.ulSectionGlobalDataLength=%lu\n", data.ulSectionGlobalDataLength); - ok_(__FILE__, line)(data.lpSectionBase != NULL, "data.lpSectionBase == NULL\n"); ok_(__FILE__, line)(data.ulSectionTotalLength > 0, "data.ulSectionTotalLength=%lu\n", data.ulSectionTotalLength); ok_(__FILE__, line)(data.hActCtx == NULL, "data.hActCtx=%p\n", data.hActCtx); @@ -1914,9 +1914,9 @@ static void test_find_progid_redirection(HANDLE handle, const GUID *clsid, const } header = (struct strsection_header*)data.lpSectionBase; + ok_(__FILE__, line)(data.lpSectionBase != NULL, "data.lpSectionBase == NULL\n"); ok_(__FILE__, line)(data.lpSectionGlobalData == (BYTE*)header + header->global_offset, "data.lpSectionGlobalData == NULL\n"); ok_(__FILE__, line)(data.ulSectionGlobalDataLength == header->global_len, "data.ulSectionGlobalDataLength=%lu\n", data.ulSectionGlobalDataLength); - ok_(__FILE__, line)(data.lpSectionBase != NULL, "data.lpSectionBase == NULL\n"); ok_(__FILE__, line)(data.ulSectionTotalLength > 0, "data.ulSectionTotalLength=%lu\n", data.ulSectionTotalLength); ok_(__FILE__, line)(data.hActCtx == NULL, "data.hActCtx=%p\n", data.hActCtx); ok_(__FILE__, line)(data.ulAssemblyRosterIndex == exid, "data.ulAssemblyRosterIndex=%lu, expected %lu\n", -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2618
From: Alex Henrie <alexhenrie24(a)gmail.com> Instead of multiplying it by 2. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54808 --- dlls/kernel32/tests/actctx.c | 24 ++++++++++++++++++++++-- dlls/ntdll/actctx.c | 2 +- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 3d547e01b19..17fd2779749 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -147,6 +147,11 @@ static const char manifest3[] = " name=\"testsurrogate\"" " runtimeVersion=\"v2.0.50727\"" " />" +" <clrSurrogate " +" clsid=\"{96666666-8888-7777-6666-555555555555}\"" +" name=\"testsurrogate\"" +" runtimeVersion=\"v2.0.50728\"" +" />" " <clrClass " " clsid=\"{22345678-1234-5678-1234-111122223333}\"" " name=\"clrclass\"" @@ -1815,9 +1820,11 @@ struct clrsurrogate_data }; static void test_find_surrogate(HANDLE handle, const GUID *clsid, const WCHAR *name, const WCHAR *version, - ULONG exid, int line) + ULONG exid, ULONG count, int line) { struct clrsurrogate_data *surrogate; + struct guidsection_header *header; + struct guid_index *index; ACTCTX_SECTION_KEYED_DATA data; BOOL ret; @@ -1871,6 +1878,19 @@ static void test_find_surrogate(HANDLE handle, const GUID *clsid, const WCHAR *n ok_(__FILE__, line)(data.hActCtx == NULL, "data.hActCtx=%p\n", data.hActCtx); ok_(__FILE__, line)(data.ulAssemblyRosterIndex == exid, "data.ulAssemblyRosterIndex=%lu, expected %lu\n", data.ulAssemblyRosterIndex, exid); + + header = (struct guidsection_header*)data.lpSectionBase; + ok(header->magic == 0x64487347, "got wrong magic 0x%08lx\n", header->magic); + ok(header->size == sizeof(*header), "got size %ld\n", header->size); + ok(header->count == count, "got count %ld, expected %ld\n", header->count, count); + ok(header->index_offset == sizeof(*header), "got index offset %lu\n", header->index_offset); + + index = (struct guid_index*)((BYTE*)data.lpSectionBase + header->index_offset); + ok(index[0].data_offset == sizeof(*header) + count * sizeof(*index), "got data offset %ld\n", index[0].data_offset); + ok(index[0].rosterindex == exid, "got roster index %ld, expected %lu\n", index[0].rosterindex, exid); + ok(index[1].data_offset == index[0].data_offset + index[0].data_len, "got data offset %lu, expected %lu\n", + index[1].data_offset, index[0].data_offset + index[0].data_len); + ok(index[1].rosterindex == exid, "got roster index %ld, expected %lu\n", index[1].rosterindex, exid); } static void test_find_progid_redirection(HANDLE handle, const GUID *clsid, const char *progid, ULONG exid, int line) @@ -2299,7 +2319,7 @@ static void test_actctx(void) test_find_progid_redirection(handle, &CLSID_clrclass, "clrprogid.4", 1, __LINE__); test_find_progid_redirection(handle, &CLSID_clrclass, "clrprogid.5", 1, __LINE__); test_find_progid_redirection(handle, &CLSID_clrclass, "clrprogid.6", 1, __LINE__); - test_find_surrogate(handle, &IID_Iiface, nameW, versionW, 1, __LINE__); + test_find_surrogate(handle, &IID_Iiface, nameW, versionW, 1, 2, __LINE__); test_find_ifaceps_redirection(handle, &IID_Iifaceps, &IID_TlibTest4, &IID_Ibifaceps, NULL, 1, __LINE__); test_find_ifaceps_redirection(handle, &IID_Iifaceps2, &IID_TlibTest4, &IID_Ibifaceps, &IID_PS32, 1, __LINE__); test_find_ifaceps_redirection(handle, &IID_Iifaceps3, &IID_TlibTest4, &IID_Ibifaceps, NULL, 1, __LINE__); diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index b275a7f5b49..3fcf5196339 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -4819,7 +4819,7 @@ static NTSTATUS build_clr_surrogate_section(ACTIVATION_CONTEXT* actctx, struct g ptrW[data->version_len/sizeof(WCHAR)] = 0; } - data_offset += index->data_offset; + data_offset += index->data_len; index++; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2618
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=131843 Your paranoid android. === w7pro64 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w864 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064v1507 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064v1809 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_2qxl (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_adm (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_tsign (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_en_AE_u8 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_ar (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_ja (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_zh_CN (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w11pro64_amd (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105
Thanks for the guidance. I've pushed a new patchset that cleans up the existing header tests a bit and adds some new index tests that demonstrate the correctness of the change to build_clr_surrogate_section. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_29873
Nikolay Sivov (@nsivov) commented about dlls/kernel32/tests/actctx.c:
ok_(__FILE__, line)(data.hActCtx == NULL, "data.hActCtx=%p\n", data.hActCtx); ok_(__FILE__, line)(data.ulAssemblyRosterIndex == exid, "data.ulAssemblyRosterIndex=%lu, expected %lu\n", data.ulAssemblyRosterIndex, exid); + + header = (struct guidsection_header*)data.lpSectionBase; + ok(header->magic == 0x64487347, "got wrong magic 0x%08lx\n", header->magic); + ok(header->size == sizeof(*header), "got size %ld\n", header->size); + ok(header->count == count, "got count %ld, expected %ld\n", header->count, count); + ok(header->index_offset == sizeof(*header), "got index offset %lu\n", header->index_offset); + + index = (struct guid_index*)((BYTE*)data.lpSectionBase + header->index_offset); + ok(index[0].data_offset == sizeof(*header) + count * sizeof(*index), "got data offset %ld\n", index[0].data_offset); + ok(index[0].rosterindex == exid, "got roster index %ld, expected %lu\n", index[0].rosterindex, exid); + ok(index[1].data_offset == index[0].data_offset + index[0].data_len, "got data offset %lu, expected %lu\n", + index[1].data_offset, index[0].data_offset + index[0].data_len); + ok(index[1].rosterindex == exid, "got roster index %ld, expected %lu\n", index[1].rosterindex, exid);
I was thinking of having some helper that would validate guid index, and another one for string index (unrelated to your fix). There is no need to pass in number of entries, it's fine to use what's in the header. Then we'd call for index validation for all sections, it's possible there are more issues like what you've found. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_29906
On Thu Apr 13 11:57:27 2023 +0000, Nikolay Sivov wrote:
I was thinking of having some helper that would validate guid index, and another one for string index (unrelated to your fix). There is no need to pass in number of entries, it's fine to use what's in the header. Then we'd call for index validation for all sections, it's possible there are more issues like what you've found. Today I tried writing generic tests to check any section header for consistency. The problem I ran into is that Windows is not consistent about where it puts blocks of data in the sections. Sometimes the blocks are not contiguous and sometimes they are out of order compared to the index. It would seem that the exact layout is unimportant.
My preference would be to fix the obviously broken code now and worry about adding tests later. If the one-line fix cannot be accepted into mainline Wine without more tests, let's put it in Wine Staging until better tests are written. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_30017
On Thu Apr 13 06:04:31 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` 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=131843 Your paranoid android. === w7pro64 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w864 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064v1507 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064v1809 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_2qxl (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_adm (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w1064_tsign (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_en_AE_u8 (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_ar (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_ja (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w10pro64_zh_CN (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 === w11pro64_amd (64 bit report) === kernel32: actctx.c:2823: Test failed: got error 14105 ``` These are pre-existing failures.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_30018
On Fri Apr 14 04:20:46 2023 +0000, Alex Henrie wrote:
Today I tried writing generic tests to check any section header for consistency. The problem I ran into is that Windows is not consistent about where it puts blocks of data in the sections. Sometimes the blocks are not contiguous and sometimes they are out of order compared to the index. It would seem that the exact layout is unimportant. My preference would be to fix the obviously broken code now and worry about adding tests later. If the one-line fix cannot be accepted into mainline Wine without more tests, let's put it in Wine Staging until better tests are written. It's not obvious, because tests are working for me without the fix somehow - if you add another clr entry, it is found correctly. I don't see why you'd mention wine-staging for this case if the fix is obvious to you, line count is not a metric to use.
I'll take a look at additional tests that I suggested. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_30043
On Fri Apr 14 07:31:35 2023 +0000, Nikolay Sivov wrote:
It's not obvious, because tests are working for me without the fix somehow - if you add another clr entry, it is found correctly. I don't see why you'd mention wine-staging for this case if the fix is obvious to you, line count is not a metric to use. I'll take a look at additional tests that I suggested. Please take a look. Testing that offset is within section bounds is enough to show what your fix changes. Note that your test is broken, because you can't have duplicate GUIDs in the same section. I'm not sure if we enforce that yet.
[0001-Some-tests.txt](/uploads/28e25c7e6a1f356cb53046ffaf9092f6/0001-Some-tests.txt) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2618#note_30547
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Marvin -
Nikolay Sivov (＠nsivov)