[PATCH] kernelbase: Add rudimentary mui resource support to loader.c
Zebediah Figura (she/her)
zfigura at codeweavers.com
Sun Jan 31 22:26:00 CST 2021
Hello Craig, thanks for the patch, and sorry for the lack of review thus
far.
I have a few high-level comments:
(1) It seems to me it should be possible to write some tests for this
behaviour, both to prove the implementation is correct and to prevent
further regressions. I think the easiest way to do this would be to add
a simple empty test DLL (for an example, see fce26e60cc2 and references
to "coinst"), to which you can add resources using UpdateResource().
When writing tests, I would recommend to keep in mind my other comments
below.
(2) Should this really happen in LoadResource()? Should it instead
happen in FindResource(), or even in LdrFindResource_U()?
(3) If yes, how does this interact with the LCID parameter to
FindResource()?
On 1/18/21 11:12 AM, Craig Schulstad wrote:
> Signed-off-by: Craig Schulstad <craigaschulstad at gmail.com>
> ---
> dlls/kernelbase/loader.c | 169 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 164 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/kernelbase/loader.c b/dlls/kernelbase/loader.c
> index fc9b0ce0083..0fbe9bcd9e8 100644
> --- a/dlls/kernelbase/loader.c
> +++ b/dlls/kernelbase/loader.c
> @@ -48,6 +48,9 @@ struct exclusive_datafile
> };
> static struct list exclusive_datafile_list = LIST_INIT( exclusive_datafile_list );
>
> +static WCHAR mui_locale[LOCALE_NAME_MAX_LENGTH];
> +static BOOL locale_found = 0;
> +static BOOL recursion_flag = 0;
>
> /***********************************************************************
> * Modules
> @@ -1011,11 +1014,122 @@ BOOL WINAPI DECLSPEC_HOTPATCH EnumResourceTypesExW( HMODULE module, ENUMRESTYPEP
> return ret;
> }
>
> +/***********************************************************************/
> +/* get_mui - Acquire an MUI module for the associated resource */
> +/***********************************************************************/
> +
> +HMODULE get_mui(HMODULE module)
> +
> +{
> +
> + HMODULE mui_module = NULL;
> +
> + WCHAR module_name[MAX_PATH], mui_name[MAX_PATH];
> +
> + INT i, j, k, l;
> +
> + /* Initialize the work strings */
> +
> + for (i = 0; i < MAX_PATH; i++) {
> + module_name[i] = 0;
> + mui_name[i] = 0;
> + }
> +
> + /* Note - the reference to the Windows file name for an "MUI" file has a structure such as */
> + /* "C:\Program Files\Application Directory\xx-XX\Application.exe.mui"; however, in testing */
> + /* out the usage of the "GetModuleFileNameW" function, it was determined that it works with */
> + /* a relative Linux file structure such as "xx-XX/Application.exe.mui". */
> +
> + /* Acquire the base resource file name */
This comment, like some others below, feel redundant; the next line
tells me as much.
> +
> + if (!(GetModuleFileNameW(module, module_name, MAX_PATH))) return module;
> +
> + /* Stay with the original module reference if this file is not an executable file. */
> +
> + if (!(wcsstr(module_name, L".exe"))) return module;
This is also a good thing to test. It seems at least a little surprising
that this would be true.
> +
> + /* Acquire the locale name using LCIDToLocaleName. Since this function utilizes the FindResourceExW function, this */
> + /* sets up a recursive call to this function. In order to avoid a stack overflow condition that would be caused by */
> + /* repeated calls, a flag will be set on to return back to the FindResourceExW function without again calling the */
> + /* locale acquisition function. */
> +
> + if (!(locale_found)) {
> +
> + if (recursion_flag) return module;
> +
> + recursion_flag = 1;
> +
> + LCIDToLocaleName( GetUserDefaultLCID(), mui_locale, LOCALE_NAME_MAX_LENGTH, 0 );
> +
> + recursion_flag = 0;
> +
> + locale_found = 1;
> +
> + }
This is not thread safe (it doesn't guarantee ordering); you would need
atomics.
I'm wondering if a better solution to this might be to use an internal
API in LCIDToLocaleName() [or RtlLcidToLocaleName()], and skip the MUI
logic in that case.
> +
> + /* Locate the position of the final backslash in the retrieved executable file. */
> +
> + j = 0;
> +
> + for (i = 0; i < MAX_PATH; i++) {
> +
> + if (module_name[i] == 0) break;
> +
> + if (module_name[i] == '\\') j = i;
> + }
> +
> + /* Set up the work index that will be used to extract just the executable file from the fully qualified file name. */
> +
> + k = 0;
> +
> + for (i = 0; i < MAX_PATH; i++) {
> +
> + if (module_name[i] == 0) break;
> +
> + /* If work index "j" has been set to -1, then the file portion of the qualified name has been reached and will */
> + /* be copied to the "MUI" file reference. */
> +
> + if (j < 0) {
> + mui_name[k] = module_name[i];
> + k++;
> + }
> +
> + /* When the position of the final backslash has been reached, add the locale name as the folder/directory */
> + /* containing the "MUI" file and reset work index "j" to -1. */
> +
> + if (i >= j && j > 0) {
> + for (l = 0; l < 5; l++) {
> + mui_name[k] = mui_locale[l];
> + k++;
> + }
> + mui_name[k] = '/';
> + k++;
> + j = -1;
> + }
> + }
Note that you can use (some) CRT string functions within kernelbase [and
ntdll], which may help avoid some of this logic. It's rather difficult
to read as-is.
> +
> + /* Finally, append the literal ".mui" onto the file reference. */
> +
> + wcscat(mui_name, L".mui");
> +
> + /* Now, see if there is an associated "MUI" file and if so use its handle for the module handle. */
> +
> + mui_module = LoadLibraryExW(mui_name, 0, 0);
> +
> + if (mui_module) {
> + return mui_module;
> + } else {
> + return module;
> + }
> +
> +}
> +
> +/***********************************************************************/
> +/* get_res_handle - Isolated call of the LdrFindResource function */
> +/***********************************************************************/
> +
> +HRSRC get_res_handle(HMODULE module, LPCWSTR type, LPCWSTR name, WORD lang)
>
> -/**********************************************************************
> - * FindResourceExW (kernelbase.@)
> - */
> -HRSRC WINAPI DECLSPEC_HOTPATCH FindResourceExW( HMODULE module, LPCWSTR type, LPCWSTR name, WORD lang )
> {
> NTSTATUS status;
> UNICODE_STRING nameW, typeW;
> @@ -1024,7 +1138,6 @@ HRSRC WINAPI DECLSPEC_HOTPATCH FindResourceExW( HMODULE module, LPCWSTR type, LP
>
> TRACE( "%p %s %s %04x\n", module, debugstr_w(type), debugstr_w(name), lang );
>
> - if (!module) module = GetModuleHandleW( 0 );
> nameW.Buffer = typeW.Buffer = NULL;
>
> __TRY
> @@ -1046,7 +1159,41 @@ HRSRC WINAPI DECLSPEC_HOTPATCH FindResourceExW( HMODULE module, LPCWSTR type, LP
>
> if (!IS_INTRESOURCE(nameW.Buffer)) HeapFree( GetProcessHeap(), 0, nameW.Buffer );
> if (!IS_INTRESOURCE(typeW.Buffer)) HeapFree( GetProcessHeap(), 0, typeW.Buffer );
> +
> return (HRSRC)entry;
> +
> +}
> +
> +/**********************************************************************
> + * FindResourceExW (kernelbase.@)
> + */
> +HRSRC WINAPI DECLSPEC_HOTPATCH FindResourceExW( HMODULE module, LPCWSTR type, LPCWSTR name, WORD lang )
> +{
> +
> + HRSRC rsrc;
> +
> + TRACE( "%p %s %s %04x\n", module, debugstr_w(type), debugstr_w(name), lang );
> +
> + if (!module) module = GetModuleHandleW( 0 );
> +
> + rsrc = get_res_handle(module, type, name, lang);
> +
> + if (rsrc) {
> +
> + return rsrc;
> +
> + } else {
> +
> + /* If a resource retrieval failed using the initial module value, attempt to */
> + /* locate an associated MUI file and retry the resource retrieval. */
> +
> + module = get_mui(module);
Here and below, "module" is effectively leaked. I guess this is
necessarily true, but is that module really never unloaded?
> +
> + rsrc = get_res_handle(module, type, name, lang);
> +
> + return rsrc;
> +
> + }
> }
>
>
> @@ -1074,11 +1221,23 @@ BOOL WINAPI DECLSPEC_HOTPATCH FreeResource( HGLOBAL handle )
> HGLOBAL WINAPI DECLSPEC_HOTPATCH LoadResource( HINSTANCE module, HRSRC rsrc )
> {
> void *ret;
> + HMODULE mui_module = NULL;
>
> TRACE( "%p %p\n", module, rsrc );
>
> if (!rsrc) return 0;
> if (!module) module = GetModuleHandleW( 0 );
> +
> +
> + /* Only check for an MUI reference if the resource handle value is less than the module value, */
> + /* or if an MUI reference was found and the MUI reference and handle value are larger than the */
> + /* module value for the executable file. That is a signal that the resource handle is to be */
> + /* associated with the MUI file instead of the executable file. */
> +
> + mui_module = get_mui(module);
> +
> + if (((HMODULE)rsrc < module) || ((mui_module > module) && ((HMODULE)rsrc > mui_module))) module = mui_module;
> +
This is very confusing. Why would the value of "rsrc" matter?
> if (!set_ntstatus( LdrAccessResource( module, (IMAGE_RESOURCE_DATA_ENTRY *)rsrc, &ret, NULL )))
> return 0;
> return ret;
>
More information about the wine-devel
mailing list