Re: Always create a suspended thread in CreateThread and resume it if CREATE_SUSPENDED flag is not set as Windows does

[Robert Shearman]

Dmitry Timoshkov wrote:
> Hello,
>
> there is at least one application which crashes in CreateThread due
> to a race between a creating thread and a newly created one. The problem
> is that the tid address passed to CreateThread resides in the memory
> block allocated with VirtualAlloc which gets freed once the newly created
> thread starts running, and when CreateThread code attempts to store thread
> id it crashes.
>
> It appears that Windows always creates suspended threads in CreateThread
> and a created thread gets resumed later. Here is a snippet of the log
> running a simple test program under NT strace (the test app just creates
> a thread):
>
> hThread = CreateThread(NULL, 0, thread_proc, NULL, 0, &tid);
> CloseHandle(hThread);
>
> ...
> 117 796 792 NtCreateThread (0x1f03ff, 0x0, -1, 1243704, 1244568, 1, ... 60, {796, 800}, ) == 0x0
> 118 796 792 NtRequestWaitReplyPort (40, {28, 52, new_msg, 0, 2013529912, 2147348480, 902, 2086471576} "\0\0\0\0\1\0\1\0x\1\23\0\320<\23\0<\0\0\0\34\3\0\0 \3\0\0" ... {28, 52, reply, 0, 796, 792, 2138, 0} "\0\0\0\0\1\0\1\0\0\0\0\0\320<\23\0<\0\0\0\34\3\0\0 \3\0\0" ) == 0x0
> 119 796 792 NtResumeThread (60, ... 1, ) == 0x0
> 120 796 792 NtClose (60, ... ) == 0x0
> ...
>
> Changelog:
>    Dmitry Timoshkov <dmitry(a)codeweavers.com>
>    Always create a suspended thread in CreateThread and resume it
>    if CREATE_SUSPENDED flag is not set as Windows does.
>
> --- cvs/hq/wine/dlls/kernel/thread.c	2004-09-22 16:48:59.000000000 +0900
> +++ wine/dlls/kernel/thread.c	2004-12-01 18:04:52.000000000 +0800
> @@ -164,7 +164,7 @@ HANDLE WINAPI CreateRemoteThread( HANDLE
>     if (flags & STACK_SIZE_PARAM_IS_A_RESERVATION) stack_reserve = stack;
>     else stack_commit = stack;
>
> -    status = RtlCreateUserThread( hProcess, NULL, (flags & CREATE_SUSPENDED) != 0,
> +    status = RtlCreateUserThread( hProcess, NULL, TRUE,
>                                   NULL, stack_reserve, stack_commit,
>                                   THREAD_Start, info, &handle, &client_id );
>     if (status == STATUS_SUCCESS)
> @@ -172,6 +172,19 @@ HANDLE WINAPI CreateRemoteThread( HANDLE
>         if (id) *id = (DWORD)client_id.UniqueThread;
>         if (sa && (sa->nLength >= sizeof(*sa)) && sa->bInheritHandle)
>             SetHandleInformation( handle, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT );
> +        if (!(flags & CREATE_SUSPENDED))
> +        {
> +            DWORD ret;
> +
> +            status = NtResumeThread( handle, &ret );
> +            if (status)
> +            {
> +                NtTerminateThread( handle, -1 );
> +                RtlFreeHeap( GetProcessHeap(), 0, info );
> +                SetLastError( RtlNtStatusToDosError(status) );
> +                handle = 0;
> +            }
> +        }
>     }
>     else

I think you should close the thread handle in the error case, otherwise 
it will be leaked.

Rob