CreateRemoteThread and related stuff (patch)

[Robert Shearman]

Alexander Yaworsky wrote:

>Hello
>
>This is a basic framework. Not finished, not really tested -- just stable point. It does no harm for applications I'm using. I would
>be glad to get some comments and suggestions.
>  
>
Looks good so far, apart from a few comments below.

>@@ -168,16 +137,68 @@
>  *   Success: Handle to the new thread.
>  *   Failure: NULL. Use GetLastError() to find the error cause.
>  *
>- * BUGS
>- *   Unimplemented
>  */
> HANDLE WINAPI CreateRemoteThread( HANDLE hProcess, SECURITY_ATTRIBUTES *sa, SIZE_T stack,
>                                   LPTHREAD_START_ROUTINE start, LPVOID param,
>                                   DWORD flags, LPDWORD id )
> {
>-    FIXME("(): stub, Write Me.\n");
>+    extern BOOL __wine_is_current_process( HANDLE hProcess );
>+
>+    HANDLE handle;
>+    CLIENT_ID client_id;
>+    NTSTATUS status;
>+    SIZE_T stack_reserve = 0, stack_commit = 0;
>+    struct new_thread_info *info;
>+    PRTL_THREAD_START_ROUTINE start_addr;
>+
>+    if (!(info = VirtualAllocEx( hProcess, NULL, sizeof(*info),
>+                                 MEM_COMMIT, PAGE_READWRITE )))
>+        return 0;
>+
>+    if( __wine_is_current_process( hProcess ) )
>  
>
There is no need to export __wine_is_current_process from ntdll and use 
it here when standard win32 apis will suffice.

>+    {
>+        info->func = start;
>+        info->arg  = param;
>+        start_addr = (void*) THREAD_Start;
>+    }
>+    else
>+    {
>+        struct new_thread_info local_info;
>+        DWORD written;
>+
>+        local_info.func = start;
>+        local_info.arg = param;
>+        if( ! WriteProcessMemory( hProcess, &info, &local_info,
>
This looks wrong. &info? Don't you mean just info?

> /***********************************************************************
>+ *           remote_op
>+ *
>+ */
>+NTSTATUS remote_op( HANDLE process, int type,
>+                    void* params, int params_size, void* result, int* result_size )
>+{
>+    HANDLE event;
>+    NTSTATUS status, remote_status;
>+
>+    /* send params */
>+    SERVER_START_REQ( remote_operation )
>+    {
>+        req->handle = process;
>+        req->type   = type;
>+        if (params) wine_server_add_data( req, params, params_size );
>+        if (!(status = wine_server_call( req )))
>+            event = reply->event;
>+    }
>+    SERVER_END_REQ;
>+    if (status)
>+        return status;
>+
>+    /* wait for completion */
>+    status = NtWaitForMultipleObjects( 1, &event, FALSE, FALSE, NULL );
>+    NtClose( event );
>+    if (HIWORD(status))
>+        return status;
>+
>+    /* get result */
>+    remote_status = 0; /* make compiler happy */
>+    SERVER_START_REQ( remote_operation_result )
>+    {
>+        wine_server_set_reply( req, result, *result_size );
>+        if (!(status = wine_server_call( req )))
>+        {
>+            remote_status = reply->status;
>+            if (result)
>+                *result_size = wine_server_reply_size( reply );
>+        }
>+    }
>+    SERVER_END_REQ;
>+
>+    return status? status : remote_status;
>+}
>  
>
I don't really like the idea of a multiplexer like this, but I guess 
otherwise we would end up with duplicated code.

>+
>+/* return address of internal thread start function in kernel32 */
>+DECL_HANDLER(get_kernel_thread_start)
>+{
>+    struct process *process;
>+
>+    /* because this is used for CreateRemoteThread,
>+       required access rights must be the same
>+     */
>+    if ((process = get_process_from_handle( req->handle,
>+                                            PROCESS_CREATE_THREAD
>+                                            | PROCESS_QUERY_INFORMATION
>+                                            | PROCESS_VM_OPERATION
>+                                            | PROCESS_VM_READ | PROCESS_VM_WRITE )))
>  
>
I would argue that it is perfectly alright for the access rights to be 
just PROCESS_QUERY_INFORMATION here.

>+    {
>+        reply->address = process->kernel_thread_start;
>+        release_object( process );
>+    }
>+}
>+
>+/* Accept parameters for remote operation and start it */
>+DECL_HANDLER(remote_operation)
>+{
>+    int access;
>+    struct process *process;
>+    struct event *event;
>+
>+    /* define required access rights */
>+    switch( req->type )
>+    {
>+        case REMOTE_OP_NEW_THREAD:
>+            access = PROCESS_CREATE_THREAD | PROCESS_QUERY_INFORMATION
>+                     | PROCESS_VM_OPERATION | PROCESS_VM_READ | PROCESS_VM_WRITE;
>+            break;
>+        case REMOTE_OP_VM_ALLOC:   access = PROCESS_VM_OPERATION; break;
>+        case REMOTE_OP_VM_FREE:    access = PROCESS_VM_OPERATION; break;
>+        case REMOTE_OP_VM_PROTECT: access = PROCESS_VM_OPERATION; break;
>+        case REMOTE_OP_VM_QUERY:   access = PROCESS_QUERY_INFORMATION; break;
>+        case REMOTE_OP_VM_MAP:     access = PROCESS_VM_OPERATION; break;
>+        case REMOTE_OP_VM_UNMAP:   access = PROCESS_VM_OPERATION; break;
>+        case REMOTE_OP_VM_FLUSH:   access = PROCESS_VM_OPERATION; break; /* FIXME: is access right? */
>+        default:
>+            set_error( STATUS_INVALID_PARAMETER );
>+            return;
>+    }
>+
>+    /* get process object */
>+    if (!(process = get_process_from_handle( req->handle, access ))) return;
>+
>+    /* dispose result data buffer if allocated */
>+    if (current->result_data)
>+    {
>+        free( current->result_data );
>+        current->result_data = NULL;
>+    }
>+
>+    /* create event object */
>+    reply->event = NULL;
>+    if (current->result_event)
>+    {
>+        release_object( current->result_event );
>+        current->result_event = NULL;
>+    }
>+    if (!(event = create_event( NULL, 0, 1, 0 )))
>+        goto error;
>+
>+    if (!(reply->event = alloc_handle( current->process, event, EVENT_ALL_ACCESS, FALSE )))
>+        goto error;
>+
>+    /* FIXME: pass somehow operation type, params and originator thread id
>+       for some thread in the process and force execution of operation */
>+    set_error( STATUS_NOT_IMPLEMENTED );
>+
>+    /* save event object in thread structure for future set operation;
>+       we do not release it here */
>+    current->result_event = event;
>+    release_object( process );
>+    return;
>+
>+error:
>+    if (reply->event) close_handle( current->process, reply->event, NULL );
>+    if (event) release_object( event );
>+    release_object( process );
>+}
>  
>
Is it really necessary to perform the action required for remote 
invocation of some operation asynchronously? It seems to add a lot of 
unneeded complexity.

>+/* save result of remote operation and wakeup originator */
>+DECL_HANDLER(remote_operation_complete)
>+{
>+    struct thread *thread = get_thread_from_id( req->originator );
>+
>+    if (!thread) return;
>+
>+    /* save status */
>+    thread->remote_status = req->status;
>+
>+    /* allocate buffer for result data, if required */
>+    if (thread->result_data)
>+    {
>+        free( thread->result_data );
>+        thread->result_data = NULL;
>+    }
>+    if ((thread->result_size = get_req_data_size()))
>+    {
>+        if ((thread->result_data = mem_alloc( thread->result_size )))
>+            memcpy( thread->result_data, get_req_data(), thread->result_size );
>+        else
>+            thread->remote_status = get_error();
>+    }
>+
>+    /* set event */
>+    if (thread->result_event)
>+    {
>+        set_event( thread->result_event );
>+        release_object( thread->result_event );
>+        thread->result_event = NULL;
>+    }
>+    release_object( thread );
>+}
>+
>+/* return status and result data from remote opertaion */
>+DECL_HANDLER(remote_operation_result)
>+{
>+    reply->status = current->remote_status;
>+
>+    if (current->result_data)
>+    {
>+        void *result = current->result_data;
>+        current->result_data = NULL;
>+        set_reply_data_ptr( result, current->result_size );
>     }
> }
>  
>

Rob