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 );elsethread->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