[PATCH v2 1/4] kernelbase: Support PROC_THREAD_ATTRIBUTE_PARENT_PROCESS in CreateProcessInternalW().
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - don't use INVALID_HANDLE_VALUE to return an error on zero parent.
dlls/kernelbase/process.c | 88 ++++++++++++++++++++++++++------------- 1 file changed, 59 insertions(+), 29 deletions(-)
diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 90ea299416..d6106eb5f6 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -244,7 +244,7 @@ static RTL_USER_PROCESS_PARAMETERS *create_process_params( const WCHAR *filename */ static NTSTATUS create_nt_process( SECURITY_ATTRIBUTES *psa, SECURITY_ATTRIBUTES *tsa, BOOL inherit, DWORD flags, RTL_USER_PROCESS_PARAMETERS *params, - RTL_USER_PROCESS_INFORMATION *info ) + RTL_USER_PROCESS_INFORMATION *info, HANDLE parent ) { NTSTATUS status; UNICODE_STRING nameW; @@ -257,7 +257,7 @@ static NTSTATUS create_nt_process( SECURITY_ATTRIBUTES *psa, SECURITY_ATTRIBUTES status = RtlCreateUserProcess( &nameW, OBJ_CASE_INSENSITIVE, params, psa ? psa->lpSecurityDescriptor : NULL, tsa ? tsa->lpSecurityDescriptor : NULL, - 0, inherit, 0, 0, info ); + parent, inherit, 0, 0, info ); RtlFreeUnicodeString( &nameW ); } return status; @@ -288,7 +288,7 @@ static NTSTATUS create_vdm_process( SECURITY_ATTRIBUTES *psa, SECURITY_ATTRIBUTE winevdm, params->ImagePathName.Buffer, params->CommandLine.Buffer ); RtlInitUnicodeString( ¶ms->ImagePathName, winevdm ); RtlInitUnicodeString( ¶ms->CommandLine, newcmdline ); - status = create_nt_process( psa, tsa, inherit, flags, params, info ); + status = create_nt_process( psa, tsa, inherit, flags, params, info, NULL ); HeapFree( GetProcessHeap(), 0, newcmdline ); return status; } @@ -316,7 +316,7 @@ static NTSTATUS create_cmd_process( SECURITY_ATTRIBUTES *psa, SECURITY_ATTRIBUTE swprintf( newcmdline, len, L"%s /s/c "%s"", comspec, params->CommandLine.Buffer ); RtlInitUnicodeString( ¶ms->ImagePathName, comspec ); RtlInitUnicodeString( ¶ms->CommandLine, newcmdline ); - status = create_nt_process( psa, tsa, inherit, flags, params, info ); + status = create_nt_process( psa, tsa, inherit, flags, params, info, NULL ); RtlFreeHeap( GetProcessHeap(), 0, newcmdline ); return status; } @@ -368,7 +368,6 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessAsUserW( HANDLE token, const WCHAR *a inherit, flags, env, cur_dir, startup_info, info, NULL ); }
- /********************************************************************** * CreateProcessInternalA (kernelbase.@) */ @@ -382,7 +381,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalA( HANDLE token, const char * BOOL ret = FALSE; WCHAR *app_nameW = NULL, *cmd_lineW = NULL, *cur_dirW = NULL; UNICODE_STRING desktopW, titleW; - STARTUPINFOW infoW; + STARTUPINFOEXW infoW;
desktopW.Buffer = NULL; titleW.Buffer = NULL; @@ -393,12 +392,15 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalA( HANDLE token, const char * if (startup_info->lpDesktop) RtlCreateUnicodeStringFromAsciiz( &desktopW, startup_info->lpDesktop ); if (startup_info->lpTitle) RtlCreateUnicodeStringFromAsciiz( &titleW, startup_info->lpTitle );
- memcpy( &infoW, startup_info, sizeof(infoW) ); - infoW.lpDesktop = desktopW.Buffer; - infoW.lpTitle = titleW.Buffer; + memcpy( &infoW.StartupInfo, startup_info, sizeof(infoW.StartupInfo) ); + infoW.StartupInfo.lpDesktop = desktopW.Buffer; + infoW.StartupInfo.lpTitle = titleW.Buffer; + + if (flags & EXTENDED_STARTUPINFO_PRESENT) + infoW.lpAttributeList = ((STARTUPINFOEXW *)startup_info)->lpAttributeList;
ret = CreateProcessInternalW( token, app_nameW, cmd_lineW, process_attr, thread_attr, - inherit, flags, env, cur_dirW, &infoW, info, new_token ); + inherit, flags, env, cur_dirW, (STARTUPINFOW *)&infoW, info, new_token ); done: RtlFreeHeap( GetProcessHeap(), 0, app_nameW ); RtlFreeHeap( GetProcessHeap(), 0, cmd_lineW ); @@ -408,6 +410,22 @@ done: return ret; }
+struct proc_thread_attr +{ + DWORD_PTR attr; + SIZE_T size; + void *value; +}; + +struct _PROC_THREAD_ATTRIBUTE_LIST +{ + DWORD mask; /* bitmask of items in list */ + DWORD size; /* max number of items in list */ + DWORD count; /* number of items in list */ + DWORD pad; + DWORD_PTR unk; + struct proc_thread_attr attrs[1]; +};
/********************************************************************** * CreateProcessInternalW (kernelbase.@) @@ -423,6 +441,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalW( HANDLE token, const WCHAR WCHAR *p, *tidy_cmdline = cmd_line; RTL_USER_PROCESS_PARAMETERS *params = NULL; RTL_USER_PROCESS_INFORMATION rtl_info; + HANDLE parent = NULL; NTSTATUS status;
/* Process the AppName and/or CmdLine to get module name and path */ @@ -473,7 +492,36 @@ BOOL WINAPI DECLSPEC_HOTPATCH CreateProcessInternalW( HANDLE token, const WCHAR goto done; }
- status = create_nt_process( process_attr, thread_attr, inherit, flags, params, &rtl_info ); + if (flags & EXTENDED_STARTUPINFO_PRESENT) + { + struct _PROC_THREAD_ATTRIBUTE_LIST *attrs = + (struct _PROC_THREAD_ATTRIBUTE_LIST *)((STARTUPINFOEXW *)startup_info)->lpAttributeList; + unsigned int i; + + if (attrs) + { + for (i = 0; i < attrs->count; ++i) + { + switch(attrs->attrs[i].attr) + { + case PROC_THREAD_ATTRIBUTE_PARENT_PROCESS: + parent = *(HANDLE *)attrs->attrs[i].value; + TRACE("PROC_THREAD_ATTRIBUTE_PARENT_PROCESS parent %p.\n", parent); + if (!parent) + { + status = STATUS_INVALID_HANDLE; + goto done; + } + break; + default: + FIXME("Unsupported attribute %#lx.\n", attrs->attrs[i].attr); + break; + } + } + } + } + + status = create_nt_process( process_attr, thread_attr, inherit, flags, params, &rtl_info, parent ); switch (status) { case STATUS_SUCCESS: @@ -1301,24 +1349,6 @@ BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentVariableW( LPCWSTR name, LPCWSTR val * Process/thread attribute lists ***********************************************************************/
- -struct proc_thread_attr -{ - DWORD_PTR attr; - SIZE_T size; - void *value; -}; - -struct _PROC_THREAD_ATTRIBUTE_LIST -{ - DWORD mask; /* bitmask of items in list */ - DWORD size; /* max number of items in list */ - DWORD count; /* number of items in list */ - DWORD pad; - DWORD_PTR unk; - struct proc_thread_attr attrs[1]; -}; - /*********************************************************************** * InitializeProcThreadAttributeList (kernelbase.@) */
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47817 Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - don't use arbitrary thread as parent.
dlls/kernel32/tests/process.c | 4 ++-- dlls/ntdll/process.c | 11 +++++++++-- include/wine/server_protocol.h | 4 +++- server/console.c | 32 ++++++++++++++++---------------- server/process.c | 30 ++++++++++++++++++++++++------ server/process.h | 3 ++- server/protocol.def | 1 + server/request.h | 17 +++++++++-------- server/trace.c | 3 ++- server/user.h | 3 ++- server/winstation.c | 24 ++++++++++++++++-------- 11 files changed, 86 insertions(+), 46 deletions(-)
diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 3efbfa2402..6d7a9a74c3 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -3874,14 +3874,14 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe)
memset(&parent_data, 0, sizeof(parent_data)); ret = ReadFile(read_pipe, &parent_data, sizeof(parent_data), &size, NULL); - todo_wine_if(level == 2) ok((level == 2 && ret) || (level == 1 && !ret && GetLastError() == ERROR_INVALID_HANDLE), + ok((level == 2 && ret) || (level == 1 && !ret && GetLastError() == ERROR_INVALID_HANDLE), "Got unexpected ret %#x, level %u, GetLastError() %u.\n", ret, level, GetLastError()); }
if (level == 2) { - todo_wine ok(parent_id == parent_data.parent_id, "Got parent id %u, parent_data.parent_id %u.\n", + ok(parent_id == parent_data.parent_id, "Got parent id %u, parent_data.parent_id %u.\n", parent_id, parent_data.parent_id); return; } diff --git a/dlls/ntdll/process.c b/dlls/ntdll/process.c index 52d7ea429e..5d75a27e97 100644 --- a/dlls/ntdll/process.c +++ b/dlls/ntdll/process.c @@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
RtlNormalizeProcessParams( params );
- TRACE( "%s image %s cmdline %s\n", debugstr_us( path ), - debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine )); + TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ), + debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ), parent); + + if (parent == INVALID_HANDLE_VALUE) + { + memset(info, 0, sizeof(*info)); + return STATUS_INVALID_HANDLE; + }
if ((status = get_pe_file_info( path, attributes, &file_handle, &pe_info ))) { @@ -1709,6 +1715,7 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
SERVER_START_REQ( new_process ) { + req->parent_process = wine_server_obj_handle(parent); req->inherit_all = inherit; req->create_flags = params->DebugFlags; /* hack: creation flags stored in DebugFlags for now */ req->socket_fd = socketfd[1]; diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index aaa5fd2e33..98ecd98b08 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -769,6 +769,7 @@ struct rawinput_device struct new_process_request { struct request_header __header; + obj_handle_t parent_process; int inherit_all; unsigned int create_flags; int socket_fd; @@ -779,6 +780,7 @@ struct new_process_request /* VARARG(objattr,object_attributes); */ /* VARARG(info,startup_info,info_size); */ /* VARARG(env,unicode_str); */ + char __pad_44[4]; }; struct new_process_reply { @@ -6702,6 +6704,6 @@ union generic_reply struct resume_process_reply resume_process_reply; };
-#define SERVER_PROTOCOL_VERSION 593 +#define SERVER_PROTOCOL_VERSION 594
#endif /* __WINE_WINE_SERVER_PROTOCOL_H */ diff --git a/server/console.c b/server/console.c index 59f8843a75..691a0bf05a 100644 --- a/server/console.c +++ b/server/console.c @@ -504,37 +504,37 @@ int free_console( struct process *process ) * 2/ parent is a renderer which launches process, and process should attach to the console * rendered by parent */ -void inherit_console(struct thread *parent_thread, struct process *process, obj_handle_t hconin) +void inherit_console(struct thread *parent_thread, struct process *parent, struct process *process, + obj_handle_t hconin) { int done = 0; - struct process* parent = parent_thread->process;
/* if parent is a renderer, then attach current process to its console * a bit hacky.... */ - if (hconin) + if (hconin && parent_thread) { - struct console_input* console; + struct console_input *console;
/* FIXME: should we check some access rights ? */ - if ((console = (struct console_input*)get_handle_obj( parent, hconin, - 0, &console_input_ops ))) - { + if ((console = (struct console_input *)get_handle_obj( parent, hconin, + 0, &console_input_ops ))) + { if (console->renderer == parent_thread) - { - process->console = (struct console_input*)grab_object( console ); - process->console->num_proc++; - done = 1; - } - release_object( console ); - } + { + process->console = (struct console_input*)grab_object( console ); + process->console->num_proc++; + done = 1; + } + release_object( console ); + } else clear_error(); /* ignore error */ } /* otherwise, if parent has a console, attach child to this console */ if (!done && parent->console) { - process->console = (struct console_input*)grab_object( parent->console ); - process->console->num_proc++; + process->console = (struct console_input*)grab_object( parent->console ); + process->console->num_proc++; } }
diff --git a/server/process.c b/server/process.c index 16bb5d57e7..00ea45b068 100644 --- a/server/process.c +++ b/server/process.c @@ -1117,6 +1117,7 @@ DECL_HANDLER(new_process) const struct object_attributes *objattr = get_req_object_attributes( &sd, &name, NULL ); struct process *process = NULL; struct process *parent = current->process; + struct thread *parent_thread = current; int socket_fd = thread_get_inflight_fd( current, req->socket_fd );
if (socket_fd == -1) @@ -1148,11 +1149,26 @@ DECL_HANDLER(new_process) return; }
+ if (req->parent_process) + { + if (!(parent = get_process_from_handle( req->parent_process, PROCESS_CREATE_PROCESS))) + { + set_error(STATUS_INVALID_HANDLE); + close(socket_fd); + return; + } + parent_thread = NULL; + } + if (parent->job && (req->create_flags & CREATE_BREAKAWAY_FROM_JOB) && !(parent->job->limit_flags & (JOB_OBJECT_LIMIT_BREAKAWAY_OK | JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK))) { set_error( STATUS_ACCESS_DENIED ); close( socket_fd ); + + if (req->parent_process) + release_object(parent); + return; }
@@ -1222,7 +1238,7 @@ DECL_HANDLER(new_process) }
/* connect to the window station */ - connect_process_winstation( process, current ); + connect_process_winstation( process, parent_thread, parent );
/* set the process console */ if (!(req->create_flags & (DETACHED_PROCESS | CREATE_NEW_CONSOLE))) @@ -1231,7 +1247,7 @@ DECL_HANDLER(new_process) * like if hConOut and hConIn are console handles, then they should be on the same * physical console */ - inherit_console( current, process, req->inherit_all ? info->data->hstdin : 0 ); + inherit_console( parent_thread, parent, process, req->inherit_all ? info->data->hstdin : 0 ); }
if (!req->inherit_all && !(req->create_flags & CREATE_NEW_CONSOLE)) @@ -1246,16 +1262,15 @@ DECL_HANDLER(new_process) if (get_error() == STATUS_INVALID_HANDLE || get_error() == STATUS_OBJECT_TYPE_MISMATCH) clear_error(); } - /* attach to the debugger if requested */ if (req->create_flags & (DEBUG_PROCESS | DEBUG_ONLY_THIS_PROCESS)) { set_process_debugger( process, current ); process->debug_children = !(req->create_flags & DEBUG_ONLY_THIS_PROCESS); } - else if (parent->debugger && parent->debug_children) + else if (current->process->debugger && current->process->debug_children) { - set_process_debugger( process, parent->debugger ); + set_process_debugger( process, current->process->debugger ); /* debug_children is set to 1 by default */ }
@@ -1265,9 +1280,12 @@ DECL_HANDLER(new_process) info->process = (struct process *)grab_object( process ); reply->info = alloc_handle( current->process, info, SYNCHRONIZE, 0 ); reply->pid = get_process_id( process ); - reply->handle = alloc_handle_no_access_check( parent, process, req->access, objattr->attributes ); + reply->handle = alloc_handle_no_access_check( current->process, process, req->access, objattr->attributes );
done: + if (req->parent_process) + release_object(parent); + if (process) release_object( process ); release_object( info ); } diff --git a/server/process.h b/server/process.h index 20ff6beda6..d8453eeaf2 100644 --- a/server/process.h +++ b/server/process.h @@ -141,7 +141,8 @@ extern struct process_snapshot *process_snap( int *count ); extern void enum_processes( int (*cb)(struct process*, void*), void *user);
/* console functions */ -extern void inherit_console(struct thread *parent_thread, struct process *process, obj_handle_t hconin); +extern void inherit_console(struct thread *parent_thread, struct process *parent, + struct process *process, obj_handle_t hconin); extern int free_console( struct process *process ); extern struct thread *console_get_renderer( struct console_input *console );
diff --git a/server/protocol.def b/server/protocol.def index 1cb1fea602..7f9ec3a149 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -783,6 +783,7 @@ struct rawinput_device
/* Create a new process from the context of the parent */ @REQ(new_process) + obj_handle_t parent_process; /* parent process */ int inherit_all; /* inherit all handles from parent */ unsigned int create_flags; /* creation flags */ int socket_fd; /* file descriptor for process socket */ diff --git a/server/request.h b/server/request.h index 90a3180a6c..9f36bcb711 100644 --- a/server/request.h +++ b/server/request.h @@ -745,14 +745,15 @@ C_ASSERT( sizeof(unsigned char) == 1 ); C_ASSERT( sizeof(unsigned int) == 4 ); C_ASSERT( sizeof(unsigned short) == 2 ); C_ASSERT( sizeof(user_handle_t) == 4 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, inherit_all) == 12 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, create_flags) == 16 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, socket_fd) == 20 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, exe_file) == 24 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, access) == 28 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, cpu) == 32 ); -C_ASSERT( FIELD_OFFSET(struct new_process_request, info_size) == 36 ); -C_ASSERT( sizeof(struct new_process_request) == 40 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, parent_process) == 12 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, inherit_all) == 16 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, create_flags) == 20 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, socket_fd) == 24 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, exe_file) == 28 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, access) == 32 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, cpu) == 36 ); +C_ASSERT( FIELD_OFFSET(struct new_process_request, info_size) == 40 ); +C_ASSERT( sizeof(struct new_process_request) == 48 ); C_ASSERT( FIELD_OFFSET(struct new_process_reply, info) == 8 ); C_ASSERT( FIELD_OFFSET(struct new_process_reply, pid) == 12 ); C_ASSERT( FIELD_OFFSET(struct new_process_reply, handle) == 16 ); diff --git a/server/trace.c b/server/trace.c index 411369a4f6..026aba9c50 100644 --- a/server/trace.c +++ b/server/trace.c @@ -1243,7 +1243,8 @@ typedef void (*dump_func)( const void *req );
static void dump_new_process_request( const struct new_process_request *req ) { - fprintf( stderr, " inherit_all=%d", req->inherit_all ); + fprintf( stderr, " parent_process=%04x", req->parent_process ); + fprintf( stderr, ", inherit_all=%d", req->inherit_all ); fprintf( stderr, ", create_flags=%08x", req->create_flags ); fprintf( stderr, ", socket_fd=%d", req->socket_fd ); fprintf( stderr, ", exe_file=%04x", req->exe_file ); diff --git a/server/user.h b/server/user.h index eb1b7ce1e4..35184d8780 100644 --- a/server/user.h +++ b/server/user.h @@ -183,7 +183,8 @@ extern client_ptr_t get_class_client_ptr( struct window_class *class ); extern struct desktop *get_desktop_obj( struct process *process, obj_handle_t handle, unsigned int access ); extern struct winstation *get_process_winstation( struct process *process, unsigned int access ); extern struct desktop *get_thread_desktop( struct thread *thread, unsigned int access ); -extern void connect_process_winstation( struct process *process, struct thread *parent ); +extern void connect_process_winstation( struct process *process, struct thread *parent_thread, + struct process *parent_process ); extern void set_process_default_desktop( struct process *process, struct desktop *desktop, obj_handle_t handle ); extern void close_process_desktop( struct process *process ); diff --git a/server/winstation.c b/server/winstation.c index a09ca03e3b..f7932a9dc6 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -372,7 +372,8 @@ void set_process_default_desktop( struct process *process, struct desktop *deskt }
/* connect a process to its window station */ -void connect_process_winstation( struct process *process, struct thread *parent ) +void connect_process_winstation( struct process *process, struct thread *parent_thread, + struct process *parent_process) { struct winstation *winstation = NULL; struct desktop *desktop = NULL; @@ -383,9 +384,9 @@ void connect_process_winstation( struct process *process, struct thread *parent { winstation = (struct winstation *)get_handle_obj( process, handle, 0, &winstation_ops ); } - else if (parent && parent->process->winstation) + else if (parent_process->winstation) { - handle = duplicate_handle( parent->process, parent->process->winstation, + handle = duplicate_handle( parent_process, parent_process->winstation, process, 0, 0, DUP_HANDLE_SAME_ACCESS ); winstation = (struct winstation *)get_handle_obj( process, handle, 0, &winstation_ops ); } @@ -397,14 +398,21 @@ void connect_process_winstation( struct process *process, struct thread *parent desktop = get_desktop_obj( process, handle, 0 ); if (!desktop || desktop->winstation != winstation) goto done; } - else if (parent && parent->desktop) + else { - desktop = get_desktop_obj( parent->process, parent->desktop, 0 ); + if (parent_thread && parent_thread->desktop) + handle = parent_thread->desktop; + else if (parent_process->desktop) + handle = parent_process->desktop; + else + goto done; + + desktop = get_desktop_obj( parent_process, handle, 0 ); + if (!desktop || desktop->winstation != winstation) goto done; - handle = duplicate_handle( parent->process, parent->desktop, - process, 0, 0, DUP_HANDLE_SAME_ACCESS ); - }
+ handle = duplicate_handle( parent_process, handle, process, 0, 0, DUP_HANDLE_SAME_ACCESS ); + } if (handle) set_process_default_desktop( process, desktop, handle );
done:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=61718
Your paranoid android.
=== w1064v1507 (32 bit report) ===
Report errors: kernel32:process is missing some failure messages
Paul Gofman gofmanp@gmail.com writes:
@@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
RtlNormalizeProcessParams( params );
- TRACE( "%s image %s cmdline %s\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ));
- TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ), parent);- if (parent == INVALID_HANDLE_VALUE)
- {
memset(info, 0, sizeof(*info));return STATUS_INVALID_HANDLE;- }
You are still using INVALID_HANDLE_VALUE here, I don't think that makes sense. The check for an invalid handle should use a truly invalid value like 0xdeadbeef.
It may be interesting to test what happens with GetCurrentProcess(), and also some other handle pointing to the current process, but that should be separate from the generic invalid handle test.
On 12/12/19 11:35, Alexandre Julliard wrote:
Paul Gofman gofmanp@gmail.com writes:
@@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
RtlNormalizeProcessParams( params );
- TRACE( "%s image %s cmdline %s\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ));
- TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ), parent);- if (parent == INVALID_HANDLE_VALUE)
- {
memset(info, 0, sizeof(*info));return STATUS_INVALID_HANDLE;- }
You are still using INVALID_HANDLE_VALUE here, I don't think that makes sense. The check for an invalid handle should use a truly invalid value like 0xdeadbeef.
It may be interesting to test what happens with GetCurrentProcess(), and also some other handle pointing to the current process, but that should be separate from the generic invalid handle test.
The check for invalid handle here is not for something explicitly assigned in kernelbase, CreateProcessInternalW() in my v2 patch doesn't encode any error conditions in INVALID_HANDLE_VALUE anymore. The truly invalid handles (like 0xdeadbeef) get error from server call. The same is for the other pseudo handles like that for current thread as they have wrong type. Unlike INVALID_HANDLE_VALUE which is taken as current process there. I have a test with 0xdeadbeef in the next patch which returns an error on process creation on all Windows versions as well as with these patches without any explicit checks before server call. There are also tests with INVALID_HANDLE_VALUE and GetCurrentProcess() present in the next patch which fail to create the process on Vista, w7 and w10 (and creates a process on w7u / w8), so I added the explicit check to satisfy the "mainstream" behaviour. Now I briefly checked what happens if I set a duplicated GetCurrentProcess() handle or handle from OpenProcess(..., GetCurrentProcessId()) for parent handle, and the process creation succeeds with that valid non-pseudo handle for the current process. Should I add this test?
So should I maybe move this check for 0xffffffff handle to CreateProcessInternalW(), where it checks for 0 handle already?
Paul Gofman gofmanp@gmail.com writes:
On 12/12/19 11:35, Alexandre Julliard wrote:
Paul Gofman gofmanp@gmail.com writes:
@@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
RtlNormalizeProcessParams( params );
- TRACE( "%s image %s cmdline %s\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ));
- TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ), parent);- if (parent == INVALID_HANDLE_VALUE)
- {
memset(info, 0, sizeof(*info));return STATUS_INVALID_HANDLE;- }
You are still using INVALID_HANDLE_VALUE here, I don't think that makes sense. The check for an invalid handle should use a truly invalid value like 0xdeadbeef.
It may be interesting to test what happens with GetCurrentProcess(), and also some other handle pointing to the current process, but that should be separate from the generic invalid handle test.
The check for invalid handle here is not for something explicitly assigned in kernelbase, CreateProcessInternalW() in my v2 patch doesn't encode any error conditions in INVALID_HANDLE_VALUE anymore. The truly invalid handles (like 0xdeadbeef) get error from server call. The same is for the other pseudo handles like that for current thread as they have wrong type. Unlike INVALID_HANDLE_VALUE which is taken as current process there. I have a test with 0xdeadbeef in the next patch which returns an error on process creation on all Windows versions as well as with these patches without any explicit checks before server call. There are also tests with INVALID_HANDLE_VALUE and GetCurrentProcess() present in the next patch which fail to create the process on Vista, w7 and w10 (and creates a process on w7u / w8), so I added the explicit check to satisfy the "mainstream" behaviour. Now I briefly checked what happens if I set a duplicated GetCurrentProcess() handle or handle from OpenProcess(..., GetCurrentProcessId()) for parent handle, and the process creation succeeds with that valid non-pseudo handle for the current process. Should I add this test?
So should I maybe move this check for 0xffffffff handle to CreateProcessInternalW(), where it checks for 0 handle already?
I'm not sure we need the check at all, since it works on some Windows versions, and it works for other handles to the current process. Accepting GetCurrentProcess() doesn't strike me as something that needs to be considered broken.
More to the point, INVALID_HANDLE_VALUE is meaningless with process handles. It's a value that's used with file handle APIs, but for processes, the error value is 0, and 0xffffffff is the current process pseudo-handle.
On 12/12/19 13:41, Alexandre Julliard wrote:
Paul Gofman gofmanp@gmail.com writes:
On 12/12/19 11:35, Alexandre Julliard wrote:
Paul Gofman gofmanp@gmail.com writes:
@@ -1667,8 +1667,14 @@ NTSTATUS WINAPI RtlCreateUserProcess( UNICODE_STRING *path, ULONG attributes,
RtlNormalizeProcessParams( params );
- TRACE( "%s image %s cmdline %s\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ));
- TRACE( "%s image %s cmdline %s, parent %p.\n", debugstr_us( path ),
debugstr_us( ¶ms->ImagePathName ), debugstr_us( ¶ms->CommandLine ), parent);- if (parent == INVALID_HANDLE_VALUE)
- {
memset(info, 0, sizeof(*info));return STATUS_INVALID_HANDLE;- }
You are still using INVALID_HANDLE_VALUE here, I don't think that makes sense. The check for an invalid handle should use a truly invalid value like 0xdeadbeef.
It may be interesting to test what happens with GetCurrentProcess(), and also some other handle pointing to the current process, but that should be separate from the generic invalid handle test.
The check for invalid handle here is not for something explicitly assigned in kernelbase, CreateProcessInternalW() in my v2 patch doesn't encode any error conditions in INVALID_HANDLE_VALUE anymore. The truly invalid handles (like 0xdeadbeef) get error from server call. The same is for the other pseudo handles like that for current thread as they have wrong type. Unlike INVALID_HANDLE_VALUE which is taken as current process there. I have a test with 0xdeadbeef in the next patch which returns an error on process creation on all Windows versions as well as with these patches without any explicit checks before server call. There are also tests with INVALID_HANDLE_VALUE and GetCurrentProcess() present in the next patch which fail to create the process on Vista, w7 and w10 (and creates a process on w7u / w8), so I added the explicit check to satisfy the "mainstream" behaviour. Now I briefly checked what happens if I set a duplicated GetCurrentProcess() handle or handle from OpenProcess(..., GetCurrentProcessId()) for parent handle, and the process creation succeeds with that valid non-pseudo handle for the current process. Should I add this test?
So should I maybe move this check for 0xffffffff handle to CreateProcessInternalW(), where it checks for 0 handle already?
I'm not sure we need the check at all, since it works on some Windows versions, and it works for other handles to the current process. Accepting GetCurrentProcess() doesn't strike me as something that needs to be considered broken.
More to the point, INVALID_HANDLE_VALUE is meaningless with process handles. It's a value that's used with file handle APIs, but for processes, the error value is 0, and 0xffffffff is the current process pseudo-handle.
So I will remove the check then and change what is 'broken' in the tests.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - no changes.
dlls/kernel32/tests/process.c | 87 ++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)
diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 6d7a9a74c3..4b18b70882 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -3825,7 +3825,8 @@ static void test_ProcThreadAttributeList(void) /* level 0: Main test process * level 1: Process created by level 0 process without handle inheritance * level 2: Process created by level 1 process with handle inheritance and level 0 - * process parent substitute. */ + * process parent substitute. + * level 255: Process created by level 1 process during invalid parent handles testing. */ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) { PROCESS_BASIC_INFORMATION pbi; @@ -3848,6 +3849,9 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) } parent_data;
+ if (level == 255) + return; + if (!pInitializeProcThreadAttributeList) { win_skip("No support for ProcThreadAttributeList.\n"); @@ -3891,11 +3895,92 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe)
if (level) { + HANDLE handle; SIZE_T size;
ret = pInitializeProcThreadAttributeList(NULL, 1, 0, &size); ok(!ret && GetLastError() == ERROR_INSUFFICIENT_BUFFER, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + + sprintf(buffer, ""%s" tests/process.c parent %u %p", selfname, 255, read_pipe); + +#if 0 + /* Crashes on some Windows installations, otherwise successfully creates process. */ + ret = CreateProcessA(NULL, buffer, NULL, NULL, FALSE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, (STARTUPINFOA *)&si, &info); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + ok(WaitForSingleObject(info.hProcess, 30000) == WAIT_OBJECT_0, "Child process termination\n"); + CloseHandle(info.hThread); + CloseHandle(info.hProcess); +#endif + si.lpAttributeList = heap_alloc(size); + ret = pInitializeProcThreadAttributeList(si.lpAttributeList, 1, 0, &size); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + handle = INVALID_HANDLE_VALUE; + ret = pUpdateProcThreadAttribute(si.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + &handle, sizeof(handle), NULL, NULL); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + ret = CreateProcessA(NULL, buffer, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, (STARTUPINFOA *)&si, &info); + /* Broken on w7u/w8. */ + ok((!ret && GetLastError() == ERROR_INVALID_HANDLE) || broken(ret), + "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + if (ret) + { + ok(WaitForSingleObject(info.hProcess, 30000) == WAIT_OBJECT_0, "Child process termination\n"); + CloseHandle(info.hThread); + CloseHandle(info.hProcess); + } + pDeleteProcThreadAttributeList(si.lpAttributeList); + heap_free(si.lpAttributeList); + + si.lpAttributeList = heap_alloc(size); + ret = pInitializeProcThreadAttributeList(si.lpAttributeList, 1, 0, &size); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + handle = (HANDLE)0xdeadbeef; + ret = pUpdateProcThreadAttribute(si.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + &handle, sizeof(handle), NULL, NULL); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + ret = CreateProcessA(NULL, buffer, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, (STARTUPINFOA *)&si, &info); + ok(!ret && GetLastError() == ERROR_INVALID_HANDLE, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + pDeleteProcThreadAttributeList(si.lpAttributeList); + heap_free(si.lpAttributeList); + + si.lpAttributeList = heap_alloc(size); + ret = pInitializeProcThreadAttributeList(si.lpAttributeList, 1, 0, &size); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + handle = NULL; + ret = pUpdateProcThreadAttribute(si.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + &handle, sizeof(handle), NULL, NULL); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + ret = CreateProcessA(NULL, buffer, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, (STARTUPINFOA *)&si, &info); + ok(!ret && GetLastError() == ERROR_INVALID_HANDLE, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + pDeleteProcThreadAttributeList(si.lpAttributeList); + heap_free(si.lpAttributeList); + + si.lpAttributeList = heap_alloc(size); + ret = pInitializeProcThreadAttributeList(si.lpAttributeList, 1, 0, &size); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + handle = GetCurrentProcess(); + ret = pUpdateProcThreadAttribute(si.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, + &handle, sizeof(handle), NULL, NULL); + ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + ret = CreateProcessA(NULL, buffer, NULL, NULL, TRUE, EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, (STARTUPINFOA *)&si, &info); + /* Broken on w7u/w8. */ + ok((!ret && GetLastError() == ERROR_INVALID_HANDLE) || broken(ret), + "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + if (ret) + { + ok(WaitForSingleObject(info.hProcess, 30000) == WAIT_OBJECT_0, "Child process termination\n"); + CloseHandle(info.hThread); + CloseHandle(info.hProcess); + } + pDeleteProcThreadAttributeList(si.lpAttributeList); + heap_free(si.lpAttributeList); + si.lpAttributeList = heap_alloc(size); ret = pInitializeProcThreadAttributeList(si.lpAttributeList, 1, 0, &size); ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError());
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=61719
Your paranoid android.
=== w1064v1507 (32 bit report) ===
Report errors: kernel32:process is missing some failure messages
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - added test.
dlls/kernel32/tests/process.c | 55 ++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 10 deletions(-)
diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index 4b18b70882..7ca38c63e9 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -92,6 +92,7 @@ static SIZE_T (WINAPI *pGetLargePageMinimum)(void); static BOOL (WINAPI *pInitializeProcThreadAttributeList)(struct _PROC_THREAD_ATTRIBUTE_LIST*, DWORD, DWORD, SIZE_T*); static BOOL (WINAPI *pUpdateProcThreadAttribute)(struct _PROC_THREAD_ATTRIBUTE_LIST*, DWORD, DWORD_PTR, void *,SIZE_T,void*,SIZE_T*); static void (WINAPI *pDeleteProcThreadAttributeList)(struct _PROC_THREAD_ATTRIBUTE_LIST*); +static DWORD (WINAPI *pGetFinalPathNameByHandleA)(HANDLE, LPSTR, DWORD, DWORD);
/* ############################### */ static char base[MAX_PATH]; @@ -259,6 +260,7 @@ static BOOL init(void) pInitializeProcThreadAttributeList = (void *)GetProcAddress(hkernel32, "InitializeProcThreadAttributeList"); pUpdateProcThreadAttribute = (void *)GetProcAddress(hkernel32, "UpdateProcThreadAttribute"); pDeleteProcThreadAttributeList = (void *)GetProcAddress(hkernel32, "DeleteProcThreadAttributeList"); + pGetFinalPathNameByHandleA = (void *)GetProcAddress(hkernel32, "GetFinalPathNameByHandleA");
return TRUE; } @@ -3827,7 +3829,7 @@ static void test_ProcThreadAttributeList(void) * level 2: Process created by level 1 process with handle inheritance and level 0 * process parent substitute. * level 255: Process created by level 1 process during invalid parent handles testing. */ -void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) +void test_parent_process_attribute(unsigned int level, HANDLE read_pipe, HANDLE creator_stdhandle) { PROCESS_BASIC_INFORMATION pbi; char buffer[MAX_PATH + 64]; @@ -3837,7 +3839,9 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) STARTUPINFOEXA si; DWORD parent_id; NTSTATUS status; + DWORD exit_code; ULONG pbi_size; + HANDLE hstderr; HANDLE parent; DWORD size; BOOL ret; @@ -3885,8 +3889,24 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe)
if (level == 2) { + char file_path[MAX_PATH]; + ok(parent_id == parent_data.parent_id, "Got parent id %u, parent_data.parent_id %u.\n", parent_id, parent_data.parent_id); + + GetStartupInfoA(&si.StartupInfo); + hstderr = si.StartupInfo.hStdError; + + /* On Windows, std handle values are copied from creator process but seems not to be inherited from it. + * Some operation on such handle may sometimes succeed, but various way of quering information + * from such handle suggest that the handle refers to some other object. + * Windows seem to just keep the handle value, even for invalid handle. It does not always work exacly + * like that in Wine now due to special handling of console handles in kernelbase/process.c:create_process_params() + * and initialization in dlls/msvcrt: msvcrt_init_io(). */ + ok(hstderr == creator_stdhandle, "Unexpected hstderr %p, creator_hstdhandle %p.\n", + hstderr, creator_stdhandle); + size = pGetFinalPathNameByHandleA(hstderr, file_path, sizeof(file_path), FILE_NAME_NORMALIZED); + ok(!size, "Got unexpected size %u.\n", size); return; }
@@ -3902,7 +3922,7 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) ok(!ret && GetLastError() == ERROR_INSUFFICIENT_BUFFER, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError());
- sprintf(buffer, ""%s" tests/process.c parent %u %p", selfname, 255, read_pipe); + sprintf(buffer, ""%s" tests/process.c parent %u %p %p", selfname, 255, read_pipe, NULL);
#if 0 /* Crashes on some Windows installations, otherwise successfully creates process. */ @@ -3990,9 +4010,17 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe) ret = pUpdateProcThreadAttribute(si.lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, &parent, sizeof(parent), NULL, NULL); ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); + + hstderr = CreateFileA("stderr_1.tmp", GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, &sa, + CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, NULL); + ok(hstderr != INVALID_HANDLE_VALUE, "Could not create file, GetLastError() %u.\n", GetLastError()); + si.StartupInfo.dwFlags = STARTF_USESTDHANDLES; + si.StartupInfo.hStdError = hstderr; + si.StartupInfo.hStdOutput = creator_stdhandle; }
- sprintf(buffer, ""%s" tests/process.c parent %u %p", selfname, level + 1, read_pipe); + sprintf(buffer, ""%s" tests/process.c parent %u %p %p", selfname, level + 1, read_pipe, + level ? hstderr : GetStdHandle(STD_OUTPUT_HANDLE)); ret = CreateProcessA(NULL, buffer, NULL, NULL, level == 1, level == 1 ? EXTENDED_STARTUPINFO_PRESENT : 0, NULL, NULL, (STARTUPINFOA *)&si, &info); ok(ret, "Got unexpected ret %#x, GetLastError() %u.\n", ret, GetLastError()); @@ -4010,10 +4038,16 @@ void test_parent_process_attribute(unsigned int level, HANDLE read_pipe)
/* wait for child to terminate */ ok(WaitForSingleObject(info.hProcess, 30000) == WAIT_OBJECT_0, "Child process termination\n"); + GetExitCodeProcess(info.hProcess, &exit_code); + ok(!exit_code, "Child test failed, exit_code %#x.\n", exit_code); + CloseHandle(info.hThread); CloseHandle(info.hProcess); - - if (!level) + if (level) + { + CloseHandle(hstderr); + } + else { CloseHandle(read_pipe); CloseHandle(write_pipe); @@ -4064,12 +4098,13 @@ START_TEST(process) CloseHandle(info.hThread); return; } - else if (!strcmp(myARGV[2], "parent") && myARGC >= 5) + else if (!strcmp(myARGV[2], "parent") && myARGC >= 6) { - HANDLE h; + HANDLE h1, h2;
- sscanf(myARGV[4], "%p", &h); - test_parent_process_attribute(atoi(myARGV[3]), h); + sscanf(myARGV[4], "%p", &h1); + sscanf(myARGV[5], "%p", &h2); + test_parent_process_attribute(atoi(myARGV[3]), h1, h2); return; }
@@ -4138,5 +4173,5 @@ START_TEST(process) test_jobInheritance(job); test_BreakawayOk(job); CloseHandle(job); - test_parent_process_attribute(0, NULL); + test_parent_process_attribute(0, NULL, NULL); }
-
Alexandre Julliard -
Marvin -
Paul Gofman