Re: [v2] kernel32: Read entry before LdrInitializeThunk

[Sebastian Lackner]

On 03.02.2017 11:39, Andrew Wesie wrote:
> Overwatch overwrites the PE header contents in a TLS callback. This results in
> a crash on wine, because the entry point will be incorrect in start_process.
>
> Fix the issue by reading the entry point before LdrInitializeThunk is called,
> and using this stored value in start_process instead of the PE header.
>
> Signed-off-by: Andrew Wesie <awesie(a)gmail.com>
> ---
>  dlls/kernel32/process.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> index 5a9fea2..48bb7bb 100644
> --- a/dlls/kernel32/process.c
> +++ b/dlls/kernel32/process.c
> @@ -88,6 +88,7 @@ static const BOOL is_win64 = (sizeof(void *) > sizeof(int));
>
> HMODULE kernel32_handle = 0;
>  SYSTEM_BASIC_INFORMATION system_info = { 0 };
> +LPTHREAD_START_ROUTINE entry_proc = NULL;
>
> const WCHAR *DIR_Windows = NULL;
>  const WCHAR *DIR_System = NULL;
> @@ -1085,14 +1086,7 @@ static inline DWORD call_process_entry( PEB *peb, LPTHREAD_START_ROUTINE entry )
>   */
>  static DWORD WINAPI start_process( PEB *peb )
>  {
> -    IMAGE_NT_HEADERS *nt;
> -    LPTHREAD_START_ROUTINE entry;
> -
> -    nt = RtlImageNtHeader( peb->ImageBaseAddress );
> -    entry = (LPTHREAD_START_ROUTINE)((char *)peb->ImageBaseAddress +
> -                                     nt->OptionalHeader.AddressOfEntryPoint);
> -
> -    if (!nt->OptionalHeader.AddressOfEntryPoint)
> +    if (entry_proc == (LPTHREAD_START_ROUTINE)peb->ImageBaseAddress)
>      {
>          ERR( "%s doesn't have an entry point, it cannot be executed\n",
>               debugstr_w(peb->ProcessParameters->ImagePathName.Buffer) );
> @@ -1101,11 +1095,11 @@ static DWORD WINAPI start_process( PEB *peb )
>
> if (TRACE_ON(relay))
>          DPRINTF( "%04x:Starting process %s (entryproc=%p)\n", GetCurrentThreadId(),
> -                 debugstr_w(peb->ProcessParameters->ImagePathName.Buffer), entry );
> +                 debugstr_w(peb->ProcessParameters->ImagePathName.Buffer), entry_proc );
>
> SetLastError( 0 );  /* clear error code */
>      if (peb->BeingDebugged) DbgBreakPoint();
> -    return call_process_entry( peb, entry );
> +    return call_process_entry( peb, entry_proc );
>  }
>
> @@ -1186,6 +1180,7 @@ void CDECL __wine_kernel_init(void)
>      RTL_USER_PROCESS_PARAMETERS *params = peb->ProcessParameters;
>      HANDLE boot_events[2];
>      BOOL got_environment = TRUE;
> +    IMAGE_NT_HEADERS *nt;
>
> /* Initialize everything */
>
> @@ -1299,6 +1294,10 @@ void CDECL __wine_kernel_init(void)
>
> if (!params->CurrentDirectory.Handle) chdir("/"); /* avoid locking removable devices */
>
> +    nt = RtlImageNtHeader( peb->ImageBaseAddress );
> +    entry_proc = (LPTHREAD_START_ROUTINE)((char *)peb->ImageBaseAddress +
> +                                          nt->OptionalHeader.AddressOfEntryPoint);

This will not work for .NET CLI-only images. Handling for them is still missing in Wine, but in this
case ntdll replaces the entrypoint, which means it shouldn't be stored in a static variable.

I would suggest a slightly different solution: pass the start routine as parameter to start_process().
There is no attempt to make this function compatible with Windows, so you can cache the entry
point on the ntdll side. This is similar how it is done in this draft patch:

https://github.com/wine-compholio/wine-staging/blob/master/patches/ntdll-CLI...

Nevertheless, it will require careful testing. This example shows that replacing the PE header
is valid in some situations, so just ignoring all changes probably also isn't correct. Maybe
it should use the value from some loader structure?
> +
>      LdrInitializeThunk( start_process, 0, 0, 0 );
>
> error: