[PATCH resend] ntdll: Implement JobObjectBasicProcessIdList for NtQueryInformationJobObject.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Resending as it fell off the list. Anything holding this up or anything I should improve on it? I realize the memcpy thing may be not ideal, but it avoids a pointless allocation + copy. Similar logic is done in user32's list_window_children, where it copies from user_handle_t to HWND in-place, but that can suffer from aliasing issues without memcpy since it's a different type, otherwise should compile to same code when optimized. But if that's not needed I can just use a direct assignment instead. Or is there something else about it that can be improved? dlls/kernel32/tests/process.c | 9 --------- dlls/ntdll/unix/sync.c | 31 ++++++++++++++++++++++++++++--- server/process.c | 34 ++++++++++++++++++++++++++++++++++ server/protocol.def | 9 +++++++++ 4 files changed, 71 insertions(+), 12 deletions(-) diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c index e729bca..1c056fb 100644 --- a/dlls/kernel32/tests/process.c +++ b/dlls/kernel32/tests/process.c @@ -2700,15 +2700,11 @@ static void test_QueryInformationJobObject(void) pid_list->NumberOfProcessIdsInList = 42; ret = QueryInformationJobObject(job, JobObjectBasicProcessIdList, pid_list, FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList[1]), &ret_len); - todo_wine ok(!ret, "QueryInformationJobObject expected failure\n"); - todo_wine expect_eq_d(ERROR_MORE_DATA, GetLastError()); if (ret) { - todo_wine expect_eq_d(42, pid_list->NumberOfAssignedProcesses); - todo_wine expect_eq_d(42, pid_list->NumberOfProcessIdsInList); } @@ -2723,17 +2719,12 @@ static void test_QueryInformationJobObject(void) { ULONG_PTR *list = pid_list->ProcessIdList; - todo_wine ok(ret_len == FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList[2]), "QueryInformationJobObject returned ret_len=%u\n", ret_len); - todo_wine expect_eq_d(2, pid_list->NumberOfAssignedProcesses); - todo_wine expect_eq_d(2, pid_list->NumberOfProcessIdsInList); - todo_wine expect_eq_d(pi[0].dwProcessId, list[0]); - todo_wine expect_eq_d(pi[1].dwProcessId, list[1]); } } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index a13e53a..0594a5e 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -791,11 +791,36 @@ NTSTATUS WINAPI NtQueryInformationJobObject( HANDLE handle, JOBOBJECTINFOCLASS c case JobObjectBasicProcessIdList: { JOBOBJECT_BASIC_PROCESS_ID_LIST *process = info; + DWORD count, i; if (len < sizeof(*process)) return STATUS_INFO_LENGTH_MISMATCH; - memset( process, 0, sizeof(*process) ); - if (ret_len) *ret_len = sizeof(*process); - return STATUS_SUCCESS; + + count = len - FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList); + count /= sizeof(process->ProcessIdList[0]); + + SERVER_START_REQ( query_job_pids ) + { + req->job = wine_server_user_handle(handle); + wine_server_set_reply(req, process->ProcessIdList, count * sizeof(process_id_t)); + if ((ret = wine_server_call(req)) != STATUS_SUCCESS) + return ret; + process->NumberOfAssignedProcesses = reply->count; + process->NumberOfProcessIdsInList = min(count, reply->count); + } + SERVER_END_REQ; + + if (sizeof(process_id_t) < sizeof(process->ProcessIdList[0])) + { + /* start from the end to not overwrite */ + for (i = process->NumberOfProcessIdsInList; i--;) + { + ULONG_PTR id = ((process_id_t*)(&process->ProcessIdList[0]))[i]; + memcpy(&process->ProcessIdList[i], &id, sizeof(id)); + } + } + + if (ret_len) *ret_len = (char*)(&process->ProcessIdList[process->NumberOfProcessIdsInList]) - (char*)process; + return count < process->NumberOfAssignedProcesses ? STATUS_MORE_ENTRIES : STATUS_SUCCESS; } case JobObjectExtendedLimitInformation: { diff --git a/server/process.c b/server/process.c index 0870de5..48e632a 100644 --- a/server/process.c +++ b/server/process.c @@ -286,6 +286,24 @@ static int process_in_job( struct job *job, struct process *process ) return process->job == job; } +static data_size_t query_job_pids( struct job *job, data_size_t count, data_size_t maxcount, process_id_t *pids ) +{ + struct process *process; + struct job *j; + + LIST_FOR_EACH_ENTRY( j, &job->child_job_list, struct job, parent_job_entry ) + count = query_job_pids( j, count, maxcount, pids ); + + LIST_FOR_EACH_ENTRY( process, &job->process_list, struct process, job_entry ) + { + if (process->end_time) continue; + if (count < maxcount) pids[count] = process->id; + count++; + } + + return count; +} + static void add_job_process( struct job *job, struct process *process ) { struct job *j, *common_parent; @@ -1741,6 +1759,22 @@ DECL_HANDLER(process_in_job) release_object( process ); } +/* get a list of the pids associated with the job */ +DECL_HANDLER(query_job_pids) +{ + struct job *job = get_job_obj( current->process, req->job, JOB_OBJECT_QUERY ); + process_id_t *pids; + data_size_t len; + + if (!job) return; + + reply->count = query_job_pids( job, 0, 0, NULL ); + + len = min( get_reply_max_size(), reply->count * sizeof(*pids) ); + if (len && ((pids = set_reply_data_size( len )))) + query_job_pids( job, 0, len / sizeof(*pids), pids ); +} + /* retrieve information about a job */ DECL_HANDLER(get_job_info) { diff --git a/server/protocol.def b/server/protocol.def index 133d6ad..5d97cb5 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3671,6 +3671,15 @@ struct handle_info @END +/* Query a list of pids associated with the job */ +(a)REQ(query_job_pids) + obj_handle_t job; /* handle to the job */ +(a)REPLY + data_size_t count; /* number of processes associated with the job */ + VARARG(pids,uints); /* list of pids */ +(a)END + + /* Set limit flags on a job */ @REQ(set_job_limits) obj_handle_t handle; /* handle to the job */ -- 2.31.1
On Thu, Aug 12, 2021 at 07:02:37PM +0300, Gabriel Ivăncescu wrote:
diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index a13e53a..0594a5e 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -791,11 +791,36 @@ NTSTATUS WINAPI NtQueryInformationJobObject( HANDLE handle, JOBOBJECTINFOCLASS c case JobObjectBasicProcessIdList: { JOBOBJECT_BASIC_PROCESS_ID_LIST *process = info; + DWORD count, i;
if (len < sizeof(*process)) return STATUS_INFO_LENGTH_MISMATCH; - memset( process, 0, sizeof(*process) ); - if (ret_len) *ret_len = sizeof(*process); - return STATUS_SUCCESS; + + count = len - FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList); + count /= sizeof(process->ProcessIdList[0]); + + SERVER_START_REQ( query_job_pids ) + { + req->job = wine_server_user_handle(handle); + wine_server_set_reply(req, process->ProcessIdList, count * sizeof(process_id_t)); + if ((ret = wine_server_call(req)) != STATUS_SUCCESS) + return ret;
Returning out of a SERVER_*_REQ block looks odd.
+ process->NumberOfAssignedProcesses = reply->count; + process->NumberOfProcessIdsInList = min(count, reply->count); + } + SERVER_END_REQ; + + if (sizeof(process_id_t) < sizeof(process->ProcessIdList[0])) + { + /* start from the end to not overwrite */ + for (i = process->NumberOfProcessIdsInList; i--;) + { + ULONG_PTR id = ((process_id_t*)(&process->ProcessIdList[0]))[i];
ULONG_PTR id = ((process_id_t *)(process->ProcessIdList))[i]; looks simpler to understand to me.
+ memcpy(&process->ProcessIdList[i], &id, sizeof(id));
process->ProcessIdList[i] = id; should work just fine.
+ } + } + + if (ret_len) *ret_len = (char*)(&process->ProcessIdList[process->NumberOfProcessIdsInList]) - (char*)process; + return count < process->NumberOfAssignedProcesses ? STATUS_MORE_ENTRIES : STATUS_SUCCESS; } case JobObjectExtendedLimitInformation: { diff --git a/server/process.c b/server/process.c index 0870de5..48e632a 100644 --- a/server/process.c +++ b/server/process.c @@ -286,6 +286,24 @@ static int process_in_job( struct job *job, struct process *process ) return process->job == job; }
+static data_size_t query_job_pids( struct job *job, data_size_t count, data_size_t maxcount, process_id_t *pids ) +{ + struct process *process; + struct job *j; + + LIST_FOR_EACH_ENTRY( j, &job->child_job_list, struct job, parent_job_entry ) + count = query_job_pids( j, count, maxcount, pids ); + + LIST_FOR_EACH_ENTRY( process, &job->process_list, struct process, job_entry ) + { + if (process->end_time) continue; + if (count < maxcount) pids[count] = process->id; + count++; + } + + return count; +} + static void add_job_process( struct job *job, struct process *process ) { struct job *j, *common_parent; @@ -1741,6 +1759,22 @@ DECL_HANDLER(process_in_job) release_object( process ); }
+/* get a list of the pids associated with the job */ +DECL_HANDLER(query_job_pids) +{ + struct job *job = get_job_obj( current->process, req->job, JOB_OBJECT_QUERY ); + process_id_t *pids; + data_size_t len; + + if (!job) return; + + reply->count = query_job_pids( job, 0, 0, NULL );
Doesn't this just give you job->num_processes?
+ + len = min( get_reply_max_size(), reply->count * sizeof(*pids) ); + if (len && ((pids = set_reply_data_size( len )))) + query_job_pids( job, 0, len / sizeof(*pids), pids ); +} + /* retrieve information about a job */ DECL_HANDLER(get_job_info) { diff --git a/server/protocol.def b/server/protocol.def index 133d6ad..5d97cb5 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3671,6 +3671,15 @@ struct handle_info @END
+/* Query a list of pids associated with the job */ + at REQ(query_job_pids) + obj_handle_t job; /* handle to the job */ + at REPLY + data_size_t count; /* number of processes associated with the job */ + VARARG(pids,uints); /* list of pids */ + at END +
Rather than introduce a new request, couldn't you extend the get_job_info request? Huw.
On 13/09/2021 11:37, Huw Davies wrote:
On Thu, Aug 12, 2021 at 07:02:37PM +0300, Gabriel Ivăncescu wrote:
diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index a13e53a..0594a5e 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -791,11 +791,36 @@ NTSTATUS WINAPI NtQueryInformationJobObject( HANDLE handle, JOBOBJECTINFOCLASS c case JobObjectBasicProcessIdList: { JOBOBJECT_BASIC_PROCESS_ID_LIST *process = info; + DWORD count, i;
if (len < sizeof(*process)) return STATUS_INFO_LENGTH_MISMATCH; - memset( process, 0, sizeof(*process) ); - if (ret_len) *ret_len = sizeof(*process); - return STATUS_SUCCESS; + + count = len - FIELD_OFFSET(JOBOBJECT_BASIC_PROCESS_ID_LIST, ProcessIdList); + count /= sizeof(process->ProcessIdList[0]); + + SERVER_START_REQ( query_job_pids ) + { + req->job = wine_server_user_handle(handle); + wine_server_set_reply(req, process->ProcessIdList, count * sizeof(process_id_t)); + if ((ret = wine_server_call(req)) != STATUS_SUCCESS) + return ret;
Returning out of a SERVER_*_REQ block looks odd.
+ process->NumberOfAssignedProcesses = reply->count; + process->NumberOfProcessIdsInList = min(count, reply->count); + } + SERVER_END_REQ; + + if (sizeof(process_id_t) < sizeof(process->ProcessIdList[0])) + { + /* start from the end to not overwrite */ + for (i = process->NumberOfProcessIdsInList; i--;) + { + ULONG_PTR id = ((process_id_t*)(&process->ProcessIdList[0]))[i];
ULONG_PTR id = ((process_id_t *)(process->ProcessIdList))[i]; looks simpler to understand to me.
+ memcpy(&process->ProcessIdList[i], &id, sizeof(id));
process->ProcessIdList[i] = id; should work just fine.
+ } + } + + if (ret_len) *ret_len = (char*)(&process->ProcessIdList[process->NumberOfProcessIdsInList]) - (char*)process; + return count < process->NumberOfAssignedProcesses ? STATUS_MORE_ENTRIES : STATUS_SUCCESS; } case JobObjectExtendedLimitInformation: { diff --git a/server/process.c b/server/process.c index 0870de5..48e632a 100644 --- a/server/process.c +++ b/server/process.c @@ -286,6 +286,24 @@ static int process_in_job( struct job *job, struct process *process ) return process->job == job; }
+static data_size_t query_job_pids( struct job *job, data_size_t count, data_size_t maxcount, process_id_t *pids ) +{ + struct process *process; + struct job *j; + + LIST_FOR_EACH_ENTRY( j, &job->child_job_list, struct job, parent_job_entry ) + count = query_job_pids( j, count, maxcount, pids ); + + LIST_FOR_EACH_ENTRY( process, &job->process_list, struct process, job_entry ) + { + if (process->end_time) continue; + if (count < maxcount) pids[count] = process->id; + count++; + } + + return count; +} + static void add_job_process( struct job *job, struct process *process ) { struct job *j, *common_parent; @@ -1741,6 +1759,22 @@ DECL_HANDLER(process_in_job) release_object( process ); }
+/* get a list of the pids associated with the job */ +DECL_HANDLER(query_job_pids) +{ + struct job *job = get_job_obj( current->process, req->job, JOB_OBJECT_QUERY ); + process_id_t *pids; + data_size_t len; + + if (!job) return; + + reply->count = query_job_pids( job, 0, 0, NULL );
Doesn't this just give you job->num_processes?
+ + len = min( get_reply_max_size(), reply->count * sizeof(*pids) ); + if (len && ((pids = set_reply_data_size( len )))) + query_job_pids( job, 0, len / sizeof(*pids), pids ); +} + /* retrieve information about a job */ DECL_HANDLER(get_job_info) { diff --git a/server/protocol.def b/server/protocol.def index 133d6ad..5d97cb5 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3671,6 +3671,15 @@ struct handle_info @END
+/* Query a list of pids associated with the job */ + at REQ(query_job_pids) + obj_handle_t job; /* handle to the job */ + at REPLY + data_size_t count; /* number of processes associated with the job */ + VARARG(pids,uints); /* list of pids */ + at END +
Rather than introduce a new request, couldn't you extend the get_job_info request?
Huw.
Hi Huw, Thanks for the review. Indeed, I'm not very familiar with server code, I was worried for nothing that num_processes didn't include only the active processes, but you are absolutely right, I see it now in get_job_info—and extending it makes the code even simpler. I'll send a new simpler version addressing all points. Thanks, Gabriel
participants (2)
-
Gabriel Ivăncescu -
Huw Davies