[PATCH v6 0/5] MR2555: ntdll: Support ISOLATIONAWARE_MANIFEST_RESOURCE_ID (#18889)
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=18889 -- v6: ntdll: Don't hard-code DLL manifest resource ID when looking up dependency assembly. kernel32/tests: Test loading assembly manifest resource inside dependencies. ntdll: Move ACTCTX lpResourceName validation to RtlCreateActivationContext. kernel32/tests: Test setting lpResourceName to NULL for CreateActCtxW. kernel32/tests: Remove test for ACTCTX_FLAG_HMODULE_VALID with hModule = NULL case. https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, the test scenario "ACTCTX_FLAG_HMODULE_VALID but hModule if not set" is broken and unreliable. This problem is not evident in WineHQ batch test runs; rather, the test failure seems to only be triggered when the kernel32:actctx test is run in isolation. When the flag ACTCTX_FLAG_HMODULE_VALID is specified in ACTCTX but hModule is set to NULL, CreateActCtxW() may encounter different failure modes depending on the test executable file. Error codes observed so far include ERROR_SXS_CANT_GEN_ACTCTX and ERROR_SXS_MANIFEST_TOO_BIG. When the aforementioned test reports ERROR_SXS_CANT_GEN_ACTCTX on Windows, an event is recorded in the Windows Event Log as follows: - Log Name: Application - Source: SideBySide - Event ID: 59 - Level: Error - Description: Activation context generation failed for "<path..>\kernel32_test.exe".Error in manifest or policy file "<path..>\kernel32_test.exe" on line 0. Invalid Xml syntax. It appears that the inconsistent failure was caused by Windows trying to interpret the main executable file of the current process as an XML manifest file. This fails due to one or more of the following reasons: - ERROR_SXS_CANT_GEN_ACTCTX: A valid PE executable that starts with the "MZ" signature is not a valid XML file. - ERROR_SXS_MANIFEST_TOO_BIG (or ERROR_NOT_ENOUGH_MEMORY): The executable's size may exceed the limit imposed by the manifest parser. This is much more likely for binaries with debugging symbols. Meanwhile, winetest.exe bundles a stripped version of the test executable (kernel32_test-stripped.exe), which is often smaller than the original executable (not stripped). This probably explains why the problem was not visible in batch test runs. Fix this by removing the failing test entirely. --- dlls/kernel32/tests/actctx.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index da351e70466..950e3907eb1 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -2792,17 +2792,6 @@ todo_wine { delete_manifest_file("testdep1.manifest"); delete_manifest_file("testdep2.manifest"); - /* ACTCTX_FLAG_HMODULE_VALID but hModule is not set */ - memset(&actctx, 0, sizeof(ACTCTXA)); - actctx.cbSize = sizeof(ACTCTXA); - actctx.dwFlags = ACTCTX_FLAG_HMODULE_VALID; - SetLastError(0xdeadbeef); - handle = CreateActCtxA(&actctx); - ok(handle == INVALID_HANDLE_VALUE, "got handle %p\n", handle); - todo_wine - ok(GetLastError() == ERROR_SXS_CANT_GEN_ACTCTX || broken(GetLastError() == ERROR_NOT_ENOUGH_MEMORY) /* XP, win2k3 */, - "got error %ld\n", GetLastError()); - /* create from HMODULE - resource doesn't exist, lpSource is set */ memset(&actctx, 0, sizeof(ACTCTXA)); actctx.cbSize = sizeof(ACTCTXA); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/kernel32/tests/actctx.c | 54 ++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 950e3907eb1..797bd78d7e4 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -3743,6 +3743,59 @@ static void test_manifest_in_module(void) ReleaseActCtx(handle); } +static void test_manifest_resource_name_omitted(void) +{ + WCHAR pathbuf[MAX_PATH]; + HANDLE handle; + ACTCTXW ctx; + DWORD err, len; + + memset(&ctx, 0, sizeof(ctx)); + ctx.cbSize = sizeof(ctx); + ctx.dwFlags = ACTCTX_FLAG_HMODULE_VALID; + ctx.hModule = GetModuleHandleW(NULL); + handle = CreateActCtxW(&ctx); + err = GetLastError(); + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); + todo_wine + ok(err == ERROR_RESOURCE_TYPE_NOT_FOUND, "got %lu\n", err); + + memset(&ctx, 0, sizeof(ctx)); + ctx.cbSize = sizeof(ctx); + ctx.dwFlags = ACTCTX_FLAG_HMODULE_VALID | ACTCTX_FLAG_RESOURCE_NAME_VALID; + ctx.hModule = GetModuleHandleW(NULL); + ctx.lpResourceName = NULL; + handle = CreateActCtxW(&ctx); + err = GetLastError(); + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); + todo_wine + ok(err == ERROR_INVALID_PARAMETER, "got %lu\n", err); + + len = GetModuleFileNameW(NULL, pathbuf, ARRAY_SIZE(pathbuf)); + ok(len > 0 && len < ARRAY_SIZE(pathbuf), "GetModuleFileNameW returned error %lu\n", GetLastError()); + + memset(&ctx, 0, sizeof(ctx)); + ctx.cbSize = sizeof(ctx); + ctx.lpSource = pathbuf; + ctx.dwFlags = 0; + handle = CreateActCtxW(&ctx); + err = GetLastError(); + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); + todo_wine + ok(err == ERROR_RESOURCE_TYPE_NOT_FOUND, "got %lu\n", err); + + memset(&ctx, 0, sizeof(ctx)); + ctx.cbSize = sizeof(ctx); + ctx.lpSource = pathbuf; + ctx.dwFlags = ACTCTX_FLAG_RESOURCE_NAME_VALID; + ctx.lpResourceName = NULL; + handle = CreateActCtxW(&ctx); + err = GetLastError(); + ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); + todo_wine + ok(err == ERROR_INVALID_PARAMETER, "got %lu\n", err); +} + START_TEST(actctx) { int argc; @@ -3770,6 +3823,7 @@ START_TEST(actctx) } test_manifest_in_module(); + test_manifest_resource_name_omitted(); test_actctx(); test_create_fail(); test_CreateActCtx(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This prevents passing NULL resource name to get_manifest_in_module(). --- dlls/ntdll/actctx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index b275a7f5b49..f25fd10dfef 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -2951,8 +2951,6 @@ static NTSTATUS get_manifest_in_module( struct actctx_loader* acl, struct assemb hModule, debugstr_w(filename) ); } - if (!resname) return STATUS_INVALID_PARAMETER; - info.Type = RT_MANIFEST; info.Language = lang; if (!((ULONG_PTR)resname >> 16)) @@ -5223,6 +5221,9 @@ NTSTATUS WINAPI RtlCreateActivationContext( HANDLE *handle, const void *ptr ) (pActCtx->dwFlags & ~ACTCTX_FLAGS_ALL)) return STATUS_INVALID_PARAMETER; + if ((pActCtx->dwFlags & ACTCTX_FLAG_RESOURCE_NAME_VALID) && !pActCtx->lpResourceName) + return STATUS_INVALID_PARAMETER; + if (!(actctx = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*actctx) ))) return STATUS_NO_MEMORY; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- dlls/kernel32/tests/actctx.c | 382 +++++++++++++++++++++++++++++++++++ 1 file changed, 382 insertions(+) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 797bd78d7e4..987cb20d89d 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -3371,6 +3371,39 @@ typedef struct void (WINAPI *get_path)(char *buffer, int buffer_size); } sxs_info; +struct manifest_res_spec +{ + const char *name; + LANGID lang; + const char *override_manifest; +}; + +static void add_sxs_dll_manifest(const char *pathname, const struct manifest_res_spec *res_specs, size_t num_specs, const char *manifest) +{ + HANDLE update_h; + BOOL ret; + size_t i; + + update_h = BeginUpdateResourceA(pathname, FALSE); + ok(update_h != NULL, "BeginUpdateResourceA returned error %lu.\n", GetLastError()); + + for (i = 0; i < num_specs; i++) + { + const struct manifest_res_spec *res_spec = &res_specs[i]; + const char *cur_manifest = res_spec->override_manifest ? res_spec->override_manifest : manifest; + ret = UpdateResourceA(update_h, + MAKEINTRESOURCEA(RT_MANIFEST), + res_spec->name, + res_spec->lang, + (void *)cur_manifest, + strlen(cur_manifest)); + ok(ret, "UpdateResourceA returned error %lu.\n", GetLastError()); + } + + ret = EndUpdateResourceA(update_h, FALSE); + ok(ret, "EndUpdateResourceA returned error %lu.\n", GetLastError()); +} + static BOOL fill_sxs_info(sxs_info *info, const char *temp, const char *path_dll, const char *exe_manifest, const char *dll_manifest, BOOL do_load) { BOOL success; @@ -3668,6 +3701,352 @@ cleanup: } } +#define subtest_manifest_res(d,e,r,n,l) subtest_manifest_res_(__LINE__,d,e,r,n,l) +static DWORD subtest_manifest_res_(int line, const char *manifest_exe, const char *manifest_dll, const struct manifest_res_spec *res_specs, size_t num_specs, LANGID lang) +{ + char path_tmp[MAX_PATH] = "", path_dll[MAX_PATH] = "", path_manifest_exe[MAX_PATH] = ""; + char path_tmp_lang[MAX_PATH] = ""; + static const char path_tmp_suffix[] = "winek32t\\"; + WCHAR locale_name[LOCALE_NAME_MAX_LENGTH] = {0}; + DWORD err, prefix_len; + ACTCTXA actctx; + HANDLE handle; + BOOL ret; + int r; + + prefix_len = GetTempPathA(MAX_PATH - ARRAY_SIZE(path_tmp_suffix), path_tmp); + ok_(__FILE__, line)(prefix_len > 0, "GetTempPathA returned error %lu.\n", GetLastError()); + + memcpy(&path_tmp[prefix_len], path_tmp_suffix, sizeof(path_tmp_suffix) - sizeof(*path_tmp_suffix)); + ret = CreateDirectoryA(path_tmp, NULL); + ok_(__FILE__, line)(ret || GetLastError() == ERROR_ALREADY_EXISTS, + "CreateDirectoryA returned error %lu.\n", GetLastError()); + + if (lang) + { + r = LCIDToLocaleName(MAKELCID(lang, SORT_DEFAULT), + locale_name, ARRAY_SIZE(locale_name), LOCALE_ALLOW_NEUTRAL_NAMES); + ok(r > 0, "lang 0x%04x, error %lu.\n", lang, GetLastError()); + } + + if (locale_name[0]) + { + r = snprintf(path_tmp_lang, ARRAY_SIZE(path_tmp_lang), "%s%ls\\", path_tmp, locale_name); + ok_(__FILE__, line)(r > 0 && r < ARRAY_SIZE(path_tmp_lang), "got %d\n", r); + + ret = CreateDirectoryA(path_tmp_lang, NULL); + ok_(__FILE__, line)(ret || GetLastError() == ERROR_ALREADY_EXISTS, + "CreateDirectoryA returned error %lu.\n", GetLastError()); + } + else + { + r = snprintf(path_tmp_lang, ARRAY_SIZE(path_tmp_lang), "%s", path_tmp); + ok_(__FILE__, line)(r > 0 && r < ARRAY_SIZE(path_tmp_lang), "got %d\n", r); + } + + r = snprintf(path_dll, ARRAY_SIZE(path_dll), "%s%s", path_tmp_lang, "sxs_dll.dll"); + ok_(__FILE__, line)(r > 0 && r < ARRAY_SIZE(path_dll), "got %d\n", r); + + r = snprintf(path_manifest_exe, ARRAY_SIZE(path_manifest_exe), "%s%s", path_tmp, "exe.manifest"); + ok_(__FILE__, line)(r > 0 && r < ARRAY_SIZE(path_manifest_exe), "got %d\n", r); + create_manifest_file(path_manifest_exe, manifest_exe, -1, NULL, NULL); + + extract_resource("dummy.dll", "TESTDLL", path_dll); + add_sxs_dll_manifest(path_dll, res_specs, num_specs, manifest_dll); + + memset(&actctx, 0, sizeof(actctx)); + actctx.cbSize = sizeof(actctx); + actctx.lpSource = path_manifest_exe; + actctx.lpAssemblyDirectory = path_tmp; + actctx.dwFlags = ACTCTX_FLAG_ASSEMBLY_DIRECTORY_VALID; + + SetLastError(0xccccccccUL); + handle = CreateActCtxA(&actctx); + if (handle == INVALID_HANDLE_VALUE) + { + err = GetLastError(); + ok_(__FILE__, line)(err != ERROR_SUCCESS, "got %#lx.\n", err); + } + else + { + err = ERROR_SUCCESS; + ok_(__FILE__, line)(handle != NULL, "CreateActCtxA returned %p (error %lu)\n", handle, err); + ReleaseActCtx(handle); + } + + ret = DeleteFileA(path_manifest_exe); + ok_(__FILE__, line)(ret, "DeleteFileA(%s) returned error %lu\n.", debugstr_a(path_manifest_exe), GetLastError()); + + ret = DeleteFileA(path_dll); + ok_(__FILE__, line)(ret, "DeleteFileA(%s) returned error %lu\n.", debugstr_a(path_dll), GetLastError()); + + if (locale_name[0]) + { + ret = RemoveDirectoryA(path_tmp_lang); + ok_(__FILE__, line)(ret, "RemoveDirectoryA(%s) returned error %lu\n.", debugstr_a(path_tmp_lang), GetLastError()); + } + + ret = RemoveDirectoryA(path_tmp); + ok_(__FILE__, line)(ret, "RemoveDirectoryA(%s) returned error %lu %s\n.", debugstr_a(path_tmp), GetLastError(), debugstr_a(path_tmp_lang)); + + return err; +} + +/* Test loading DLL with dependency assembly in valid manifest resource */ +static void test_valid_manifest_resources(void) +{ + static const struct manifest_res_spec valid_specs_basic[] = { + /* Test arbitrary resource ID */ + { (char *)1 }, { (char *)2 }, { (char *)3 }, { (char *)4 }, + { (char *)5 }, { (char *)6 }, { (char *)7 }, { (char *)8 }, + { (char *)9 }, { (char *)10 }, { (char *)11 }, { (char *)12 }, + { (char *)13 }, { (char *)14 }, { (char *)15 }, { (char *)16 }, + { (char *)0x1234 }, { (char *)0x89ab }, { (char *)0xffff }, + + /* Test arbitrary LANGID */ + { (char *)2, MAKELANGID(LANG_ENGLISH,SUBLANG_DEFAULT) }, + { (char *)2, MAKELANGID(LANG_FRENCH,SUBLANG_DEFAULT) }, + { (char *)2, 0x1234 }, + { (char *)2, 0xffff }, + }; + static const struct manifest_res_spec multiple_resources[] = { + /* Test multiple manifest resources coexisting inside a module */ + { (char *)2, 0 }, + { (char *)2, MAKELANGID(LANG_ENGLISH,SUBLANG_DEFAULT), wrong_manifest1 }, + { (char *)2, MAKELANGID(LANG_FRENCH,SUBLANG_DEFAULT), wrong_manifest1 }, + { (char *)3, 0, wrong_manifest1 }, + { (char *)0x1234, 0, wrong_manifest1 }, + }; + static const struct manifest_res_spec multiple_resources_lang_prio[] = { + /* Test language priority among multiple manifest resources */ + { (char *)2, MAKELANGID(LANG_INVARIANT,SUBLANG_NEUTRAL) }, + { (char *)3, 0, wrong_manifest1 }, + { (char *)3, MAKELANGID(LANG_ENGLISH,SUBLANG_DEFAULT), wrong_manifest1 }, + }; + static const struct manifest_res_spec multiple_resources_named[] = { + /* Test multiple manifest resources (ID / name) coexisting inside a module */ + { (char *)2, 0 }, + { "winetestdummy", 0, wrong_manifest1 }, + }; + DWORD err; + size_t i; + + for (i = 0; i < ARRAY_SIZE(valid_specs_basic); i++) + { + winetest_push_context("valid_specs_basic[%Iu]", i); + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, &valid_specs_basic[i], 1, 0); + todo_wine_if(valid_specs_basic[i].name != (char *)1 || valid_specs_basic[i].lang != 0) + ok(err == ERROR_SUCCESS, "got error %lu.\n", err); + winetest_pop_context(); + } + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources, ARRAY_SIZE(multiple_resources), 0); + todo_wine + ok(err == ERROR_SUCCESS, "got error %lu.\n", err); + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_lang_prio, ARRAY_SIZE(multiple_resources_lang_prio), 0); + todo_wine + ok(err == ERROR_SUCCESS, "got error %lu.\n", err); + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_named, ARRAY_SIZE(multiple_resources_named), 0); + todo_wine + ok(err == ERROR_SUCCESS, "got error %lu.\n", err); +} + +/* Test loading DLL with dependency assembly in invalid manifest resource */ +static void test_invalid_manifest_resources(void) +{ + static const struct manifest_res_spec named_resources[] = { + /* Test name-only RT_MANIFEST resources */ + { "winetestdummy" }, + { "winetestdummy", 0x1234 }, + }; + static const struct manifest_res_spec multiple_resources[] = { + /* Test multiple manifest resources coexisting inside a module */ + { (char *)2, 0, wrong_manifest1 }, + { (char *)2, MAKELANGID(LANG_ENGLISH,SUBLANG_DEFAULT) }, + { (char *)2, MAKELANGID(LANG_FRENCH,SUBLANG_DEFAULT) }, + { (char *)3, 0 }, + { (char *)0x1234, 0 }, + }; + static const struct manifest_res_spec multiple_resources_lang_prio[] = { + /* Test language priority among multiple manifest resources */ + { (char *)2, MAKELANGID(LANG_INVARIANT,SUBLANG_NEUTRAL), wrong_manifest1 }, + { (char *)3, 0 }, + { (char *)3, MAKELANGID(LANG_ENGLISH,SUBLANG_DEFAULT) }, + }; + static const struct manifest_res_spec multiple_resources_named[] = { + /* Test multiple manifest resources (ID / name) coexisting inside a module */ + { (char *)2, 0, wrong_manifest1 }, + { "winetestdummy", 0 }, + }; + DWORD err; + size_t i; + + for (i = 0; i < ARRAY_SIZE(named_resources); i++) + { + winetest_push_context("named_resources[%Iu]", i); + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, &named_resources[i], 1, 0); + ok(err == ERROR_SXS_CANT_GEN_ACTCTX, "got error %lu.\n", err); + winetest_pop_context(); + } + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources, ARRAY_SIZE(multiple_resources), 0); + ok(err == ERROR_SXS_CANT_GEN_ACTCTX, "got error %lu.\n", err); + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_lang_prio, ARRAY_SIZE(multiple_resources_lang_prio), 0); + ok(err == ERROR_SXS_CANT_GEN_ACTCTX, "got error %lu.\n", err); + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_named, ARRAY_SIZE(multiple_resources_named), 0); + ok(err == ERROR_SXS_CANT_GEN_ACTCTX, "got error %lu.\n", err); + + err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, NULL, 0, 0); + ok(err == ERROR_SXS_CANT_GEN_ACTCTX, "got error %lu.\n", err); +} + +struct u16_vec +{ + UINT16 *base; + UINT16 *end; + UINT16 *alloc_end; +}; + +static BOOL u16_vec_add_uniq(struct u16_vec *vec, UINT16 item) +{ + UINT16 *p; + + for (p = vec->base; p != vec->end; p++) + { + if (*p == item) return FALSE; + } + + if (vec->end == vec->alloc_end) + { + ok(0, "u16_vec full\n"); + return FALSE; + } + else + { + *vec->end++ = item; + return TRUE; + } +} + +static void subtest_valid_manifest_resources_locale(LANGID actctx_lang) +{ + static const char manifest_exe_fmt[] = + "<assembly xmlns=\"urn:schemas-microsoft-com:asm.v1\" manifestVersion=\"1.0\">" + "<dependency>" + "<dependentAssembly>" + "<assemblyIdentity type=\"win32\" name=\"sxs_dll\" version=\"1.0.0.0\"" + " processorArchitecture=\"" ARCH "\" publicKeyToken=\"0000000000000000\" language=\"%ls\"/>" + "</dependentAssembly>" + "</dependency>" + "</assembly>"; + static const char manifest_dll_fmt[] = + "<assembly xmlns=\"urn:schemas-microsoft-com:asm.v3\" manifestVersion=\"1.0\">" + "<assemblyIdentity type=\"win32\" name=\"sxs_dll\" version=\"1.0.0.0\"" + " processorArchitecture=\"" ARCH "\" publicKeyToken=\"0000000000000000\" language=\"%ls\"/>" + "</assembly>"; + static const char manifest_dll_nofmt[] = + "<assembly xmlns=\"urn:schemas-microsoft-com:asm.v3\" manifestVersion=\"1.0\">" + "<assemblyIdentity type=\"win32\" name=\"sxs_dll\" version=\"1.0.0.0\"" + " processorArchitecture=\"" ARCH "\" publicKeyToken=\"0000000000000000\"/>" + "</assembly>"; + char manifest_exe[1024], manifest_dll[1024]; + WCHAR locale_name[LOCALE_NAME_MAX_LENGTH]; + UINT16 langs_arr[5], *it; + struct u16_vec langs; + LANGID user_ui_lang; + int ret; + + if (actctx_lang == MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL)) + { + wcscpy(locale_name, L"*"); + strcpy(manifest_dll, manifest_dll_nofmt); + } + else + { + actctx_lang = ConvertDefaultLocale(actctx_lang); + ok(actctx_lang != MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL), + "unexpected neutral locale\n"); + ret = LCIDToLocaleName(MAKELCID(actctx_lang, SORT_DEFAULT), + locale_name, ARRAY_SIZE(locale_name), LOCALE_ALLOW_NEUTRAL_NAMES); + ok(ret > 0, "error %lu.\n", GetLastError()); + + ret = snprintf(manifest_dll, ARRAY_SIZE(manifest_dll), manifest_dll_fmt, locale_name); + ok(ret > 0 && ret < ARRAY_SIZE(manifest_dll), "ret %d.\n", ret); + } + + ret = snprintf(manifest_exe, ARRAY_SIZE(manifest_exe), manifest_exe_fmt, locale_name); + ok(ret > 0 && ret < ARRAY_SIZE(manifest_exe), "ret %d.\n", ret); + + langs.base = langs_arr; + langs.end = langs_arr; + langs.alloc_end = langs_arr + ARRAY_SIZE(langs_arr); + + user_ui_lang = GetUserDefaultUILanguage(); + u16_vec_add_uniq(&langs, MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL)); + u16_vec_add_uniq(&langs, user_ui_lang); + u16_vec_add_uniq(&langs, MAKELANGID(PRIMARYLANGID(user_ui_lang), SUBLANG_NEUTRAL)); + u16_vec_add_uniq(&langs, MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT)); + u16_vec_add_uniq(&langs, 0x1); /* least number that is a valid LANGID */ + + for (it = langs.base; it != langs.end; it++) + { + struct manifest_res_spec specs[ARRAY_SIZE(langs_arr) + 1]; + size_t num_specs; + UINT16 *it2; + DWORD err; + + winetest_push_context("langs[%Id:]", it - langs.base); + + num_specs = 0; + for (it2 = it; it2 != langs.end; it2++) + { + struct manifest_res_spec spec = {(char *)2}; + spec.lang = *it2; + if (it2 != it) spec.override_manifest = wrong_manifest1; + ok(num_specs < ARRAY_SIZE(specs), "overrun\n"); + specs[num_specs++] = spec; + } + + err = subtest_manifest_res(manifest_exe, manifest_dll, specs, num_specs, actctx_lang); + todo_wine + ok(err == ERROR_SUCCESS, "got error %lu.\n", err); + + if (winetest_debug > 1 && err != ERROR_SUCCESS) + { + for (it2 = langs.base; it2 != langs.end; it2++) + { + trace("langs[%Id] = 0x%04x %c\n", it2 - langs.base, *it2, it2 == it ? '<' : ' '); + } + } + + winetest_pop_context(); + } +} + +static void test_valid_manifest_resources_locale(void) +{ + static const LANGID langs[] = { + MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL), + MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + MAKELANGID(LANG_NEUTRAL, SUBLANG_SYS_DEFAULT), + MAKELANGID(LANG_ENGLISH, SUBLANG_NEUTRAL), + MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT), + MAKELANGID(LANG_INVARIANT, SUBLANG_NEUTRAL), + }; + size_t i; + + for (i = 0; i < ARRAY_SIZE(langs); i++) + { + winetest_push_context("[%Iu]lang=0x%04x", i, langs[i]); + subtest_valid_manifest_resources_locale(langs[i]); + winetest_pop_context(); + } +} + static void run_sxs_test(int run) { switch(run) @@ -3824,6 +4203,9 @@ START_TEST(actctx) test_manifest_in_module(); test_manifest_resource_name_omitted(); + test_valid_manifest_resources(); + test_invalid_manifest_resources(); + test_valid_manifest_resources_locale(); test_actctx(); test_create_fail(); test_CreateActCtx(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> This allows any manifest resource IDs (e.g., ISOLATIONAWARE_MANIFEST_RESOURCE_ID) to be recognized when looking up the assembly manifest of a dependency. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=18889 --- dlls/kernel32/tests/actctx.c | 8 +------- dlls/ntdll/actctx.c | 27 +++++++++++++++++++++++++-- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/dlls/kernel32/tests/actctx.c b/dlls/kernel32/tests/actctx.c index 987cb20d89d..bdd19e3ae4c 100644 --- a/dlls/kernel32/tests/actctx.c +++ b/dlls/kernel32/tests/actctx.c @@ -3835,21 +3835,17 @@ static void test_valid_manifest_resources(void) { winetest_push_context("valid_specs_basic[%Iu]", i); err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, &valid_specs_basic[i], 1, 0); - todo_wine_if(valid_specs_basic[i].name != (char *)1 || valid_specs_basic[i].lang != 0) ok(err == ERROR_SUCCESS, "got error %lu.\n", err); winetest_pop_context(); } err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources, ARRAY_SIZE(multiple_resources), 0); - todo_wine ok(err == ERROR_SUCCESS, "got error %lu.\n", err); err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_lang_prio, ARRAY_SIZE(multiple_resources_lang_prio), 0); - todo_wine ok(err == ERROR_SUCCESS, "got error %lu.\n", err); err = subtest_manifest_res(two_dll_manifest_exe, two_dll_manifest_dll, multiple_resources_named, ARRAY_SIZE(multiple_resources_named), 0); - todo_wine ok(err == ERROR_SUCCESS, "got error %lu.\n", err); } @@ -4012,7 +4008,7 @@ static void subtest_valid_manifest_resources_locale(LANGID actctx_lang) } err = subtest_manifest_res(manifest_exe, manifest_dll, specs, num_specs, actctx_lang); - todo_wine + todo_wine_if(PRIMARYLANGID(actctx_lang) != LANG_NEUTRAL && PRIMARYLANGID(actctx_lang) != LANG_INVARIANT) ok(err == ERROR_SUCCESS, "got error %lu.\n", err); if (winetest_debug > 1 && err != ERROR_SUCCESS) @@ -4147,7 +4143,6 @@ static void test_manifest_resource_name_omitted(void) handle = CreateActCtxW(&ctx); err = GetLastError(); ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); - todo_wine ok(err == ERROR_INVALID_PARAMETER, "got %lu\n", err); len = GetModuleFileNameW(NULL, pathbuf, ARRAY_SIZE(pathbuf)); @@ -4171,7 +4166,6 @@ static void test_manifest_resource_name_omitted(void) handle = CreateActCtxW(&ctx); err = GetLastError(); ok(handle == INVALID_HANDLE_VALUE, "CreateActCtxW shall fail\n"); - todo_wine ok(err == ERROR_INVALID_PARAMETER, "got %lu\n", err); } diff --git a/dlls/ntdll/actctx.c b/dlls/ntdll/actctx.c index f25fd10dfef..9c1ac5201c7 100644 --- a/dlls/ntdll/actctx.c +++ b/dlls/ntdll/actctx.c @@ -2929,6 +2929,24 @@ static NTSTATUS open_nt_file( HANDLE *handle, UNICODE_STRING *name ) FILE_SHARE_READ | FILE_SHARE_DELETE, FILE_SYNCHRONOUS_IO_ALERT ); } +static NTSTATUS find_first_manifest_resource_in_module( HANDLE hModule, const WCHAR **resname ) +{ + static const LDR_RESOURCE_INFO manifest_res_info = { RT_MANIFEST }; + const IMAGE_RESOURCE_DIRECTORY_ENTRY *entry_base, *entry; + const IMAGE_RESOURCE_DIRECTORY *resdir; + NTSTATUS status; + + status = LdrFindResourceDirectory_U( hModule, &manifest_res_info, 1, &resdir ); + if (status != STATUS_SUCCESS) return status; + + if (!resdir->NumberOfIdEntries) return STATUS_RESOURCE_NAME_NOT_FOUND; + entry_base = (const IMAGE_RESOURCE_DIRECTORY_ENTRY *)(resdir + 1); + entry = entry_base + resdir->NumberOfNamedEntries; + *resname = (const WCHAR *)(ULONG_PTR)entry->u.Id; + + return STATUS_SUCCESS; +} + static NTSTATUS get_manifest_in_module( struct actctx_loader* acl, struct assembly_identity* ai, LPCWSTR filename, LPCWSTR directory, BOOL shared, HANDLE hModule, LPCWSTR resname, ULONG lang ) @@ -2951,6 +2969,12 @@ static NTSTATUS get_manifest_in_module( struct actctx_loader* acl, struct assemb hModule, debugstr_w(filename) ); } + if (!resname) + { + status = find_first_manifest_resource_in_module( hModule, &resname ); + if (status != STATUS_SUCCESS) return status; + } + info.Type = RT_MANIFEST; info.Language = lang; if (!((ULONG_PTR)resname >> 16)) @@ -3330,8 +3354,7 @@ static NTSTATUS lookup_assembly(struct actctx_loader* acl, status = open_nt_file( &file, &nameW ); if (!status) { - status = get_manifest_in_pe_file( acl, ai, nameW.Buffer, directory, FALSE, file, - (LPCWSTR)CREATEPROCESS_MANIFEST_RESOURCE_ID, 0 ); + status = get_manifest_in_pe_file( acl, ai, nameW.Buffer, directory, FALSE, file, NULL, 0 ); NtClose( file ); if (status == STATUS_SUCCESS) break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2555
v6: - Remove a failing test - Improve commit messages a bit -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_29593
The code seems simple enough. I think it is safe to merge it as is it only a fallback when resources are not initially found the usual way by wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_29919
Hi Jinoh. Not sure why you requested review... should I remember stuff I wrote 2^4 years ago <g>? comments mostly concerning last commit: - picking first ID instead of ID 1 doesn't look fully right, at least independently of context - from MS doc, [](https://learn.microsoft.com/en-us/windows/win32/sbscs/using-side-by-side-ass...), it seems file is valid iff only one of ID 1, 2 or 3 is present (and ID up to 16 are reserved) - this should at least be tested against bogus configuration - so I wonder (I don't pretend to have a definitive insight on this, but it could make sense) if instead of [ vanishing the ID 1 and blindly using first id ], we rather should a) either move up to the callers for the ID resource value, b) or check in callers if the candidate ID value meets the caller requirements -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_30104
Hi Jinoh. Not sure why you requested review... should I remember stuff I wrote 2^4 years ago ?
Sorry. I found very few people who have worked on this. I guess I should just ask for review from julliard directly. ```sh $ git log --pretty='tformat:%an <%ae>' wine-8.6 -- dlls/ntdll/actctx.c | sort | uniq -c | sort -nr | head 69 Alexandre Julliard <julliard(a)winehq.org> 38 Nikolay Sivov <nsivov(a)codeweavers.com> 14 Eric Pouech <eric.pouech(a)wanadoo.fr> 8 Jacek Caban <jacek(a)codeweavers.com> 3 Roman Mindalev <lists(a)r000n.net> 3 Michael Stefaniuc <mstefani(a)winehq.org> 2 Zebediah Figura <z.figura12(a)gmail.com> 2 Mikolaj Zalewski <mikolajz(a)google.com> 2 Michael Müller <michael(a)fds-team.de> 2 Maarten Lankhorst <m.b.lankhorst(a)gmail.com> ```
comments mostly concerning last commit:
* picking first ID instead of ID 1 doesn't look fully right, at least independently of context
Sure it might, but it's what Windows does. The `test_valid_manifest_resources` function (from patch 4/5) has tests for arbitrary resource IDs and selection priority.
* from MS doc, , it seems file is valid iff only one of ID 1, 2 or 3 is present (and ID up to 16 are reserved)
Theoretically yes, but from testing it seems that Windows don't validate (or care about) multiple IDs at all. I guess each of them is used by an independent component, as described in https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_29411.
* this should at least be tested against bogus configuration
By configuration do you mean DLL resources, or system configuration?
* so I wonder (I don't pretend to have a definitive insight on this, but it could make sense) if instead of \[ vanishing the ID 1 and blindly using first id \], we rather should a) either move up to the callers for the ID resource value, b) or check in callers if the candidate ID value meets the caller requirements
It sounds similar to the original staging patch, https://gitlab.winehq.org/wine/wine-staging/-/blob/master/patches/ntdll-Mani.... I believe my patchset mimics Windows behavior more closely, as it has tests that back it up. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_30146
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/actctx.c:
+ MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + MAKELANGID(LANG_NEUTRAL, SUBLANG_SYS_DEFAULT), + MAKELANGID(LANG_ENGLISH, SUBLANG_NEUTRAL), + MAKELANGID(LANG_ENGLISH, SUBLANG_DEFAULT), + MAKELANGID(LANG_INVARIANT, SUBLANG_NEUTRAL), + }; + size_t i; + + for (i = 0; i < ARRAY_SIZE(langs); i++) + { + winetest_push_context("[%Iu]lang=0x%04x", i, langs[i]); + subtest_valid_manifest_resources_locale(langs[i]); + winetest_pop_context(); + } +} + All these tests seem a bit overkill to end up with the conclusion that only the first manifest resource is used. I think you should make them much simpler because with this amount of code, what they are trying to uncover becomes quite obscure.
And the remaining test failures after the last patch are also unrelated, and IIUC something instead related to loading localized assemblies, and which is commented as FIXME in `lookup_assembly`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_31774
All these tests seem a bit overkill to end up with the conclusion that only the first manifest resource is used.
That's not the only conclusion. 1. If multiple manifest resources have the same ID but different languages, their precedence is resolved in a specific order (Language Netural, UI language, UI language with sublanguage netural, US English, *then* the first LCID). 2. The original wine-staging patch only recognizes manifest resource IDs in the range `MINIMUM_RESERVED_MANIFEST_RESOURCE_ID - MAXIMUM_RESERVED_MANIFEST_RESOURCE_ID` inclusive, whereas Windows accepts arbitrary non-zero resource IDs in the range `0x0001 - 0xffff` inclusive. The tests demonstrate that the original staging patch isn't sufficient. 3. Arbitrary non-zero LCIDs are also accepted. 4. Named (as opposed to numbered) resources are ignored. All the tests together single out the only "obvious" way to choose the manifest resource. I don't think you can remove any one of them and still be able to describe Windows behavior.
I think you should make them much simpler because with this amount of code, what they are trying to uncover becomes quite obscure.
By making it simpler do you mean removing tests? - In that case, which subset of the conclusions above would you like to be removed? - Otherwise, are you saying that the declarative approach (`manifest_res_spec` arrays) is unreadable compared to imperative / procedural one? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_31884
And the remaining test failures after the last patch are also unrelated, and IIUC something instead related to loading localized assemblies, and which is commented as FIXME in `lookup_assembly`.
Well, so far I assumed that leftover `todo_wine`s are harmless. Is there any reason you'd like them removed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_31885
By making it simpler do you mean removing tests?
* In that case, which subset of the conclusions above would you like to remove? * Otherwise, are you saying that the declarative approach (`manifest_res_spec` arrays) is unreadable compared to imperative / procedural one?
I don't know, my comment is mostly based on the time it took me to read the new tests, trying to understand what they were doing, and the apparent simplicity of the fix, which basically just takes the first RT_MANIFEST resource. I think that all the detail about resource ordering seems to be a LdrFindResourceDirectory_U behavior, rather than something related to activation contexts.
Well, so far I assumed that leftover `todo_wine`s are harmless. Is there any reason you'd like them removed?
No, but given the number of tests I thought at first that it was a new corner case that was left unfixed. But it's actually unrelated, and imho also a sign that the tests could be made simpler. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_31887
By making it simpler do you mean removing tests?
* In that case, which subset of the conclusions above would you like to remove? * Otherwise, are you saying that the declarative approach (`manifest_res_spec` arrays) is unreadable compared to imperative / procedural one?
I don't know, my comment is mostly based on the time it took me to read the new tests, trying to understand what they were doing, and the apparent simplicity of the fix, which basically just takes the first RT_MANIFEST resource.
The fix could be different or even more simpler if we only assume that "the first manifest resource is used." Quoting the test descriptions above:
1. If multiple manifest resources have the same ID but different languages, their precedence is resolved in a specific order (Language Netural, UI language, UI language with sublanguage netural, US English, *then* the first LCID). 2. The original wine-staging patch only recognizes manifest resource IDs in the range `MINIMUM_RESERVED_MANIFEST_RESOURCE_ID - MAXIMUM_RESERVED_MANIFEST_RESOURCE_ID` inclusive, whereas Windows accepts arbitrary non-zero resource IDs in the range `0x0001 - 0xffff` inclusive. The tests demonstrate that the original staging patch isn't sufficient. 3. Arbitrary non-zero LCIDs are also accepted. 4. Named (as opposed to numbered) resources are ignored.
Let's try omitting each one of above. - Omitting (1), we could have hard-coded a neutral locale for `info.Language`. - Omitting (2), we could have just re-used the staging patch. - Omitting (3), we could have hard-coded the list of accepted locales. - Omitting (4), we could have ommited the line `if (!resdir->NumberOfIdEntries) return STATUS_RESOURCE_NAME_NOT_FOUND;` and `entry = entry_base + resdir->NumberOfNamedEntries;` in the implementation part as well.
I think that all the detail about resource ordering seems to be a LdrFindResourceDirectory_U behavior, rather than something related to activation contexts.
Well, so far I assumed that leftover `todo_wine`s are harmless. Is there any reason you'd like them removed?
No, but given the number of tests I thought at first that it was a new corner case that was left unfixed. But it's actually unrelated, and imho also a sign that the tests could be made simpler.
Okay, I'll just remove the tests for the English locales. That'll remove the leftover todo\_wine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2555#note_31894
participants (5)
-
eric pouech (＠epo) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Loïc Rebmeister
-
Rémi Bernon