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