Re: [PATCH v4 01/11] ntoskrnl.exe: Implement PsLookupThreadByThreadId.

[Jacek Caban]

On 4/11/19 9:45 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho
>
> dlls/ntoskrnl.exe/ntoskrnl.c        | 20 ++++++++++++++++++++
>   dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  2 +-
>   dlls/ntoskrnl.exe/tests/driver.c    | 17 +++++++++++++++++
>   include/ddk/ntifs.h                 |  1 +
>   4 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> index 861eb40429..0365634772 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> @@ -3232,6 +3232,26 @@ NTSTATUS WINAPI PsLookupProcessByProcessId(HANDLE processid, PEPROCESS *process)
>   }
>
> +/*****************************************************
> -
> -
>       PsLookupThreadByThreadId   (NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI PsLookupThreadByThreadId(HANDLE threadid, PETHREAD *thread)
> +{
> - NTSTATUS status;
> - HANDLE hThread = OpenThread( THREAD_ALL_ACCESS, FALSE, HandleToUlong(threadid) );

Please avoid Hungarian names like that, 'handle' would be preferred.
> -
> - if (!hThread)
> -
>    return STATUS_INVALID_PARAMETER;
> -
> - status = kernel_object_from_handle( hThread, PsThreadType, (void**)thread );
> -
> - ObReferenceObject( *thread );

Please use ObReferenceObjectByHandle instead. I made 
kernel_object_from_handle available for whole ntoskrnl.exe for cases 
that don't need to reference object (otherwise we'd have unnecessary 
reference/dereference sequences), but when you actually need to 
reference the object, ObReferenceObjectByHandle seems to be a better fit.
> - NtClose( hThread );
> - return status;
>
> +}
> -
> - /*****************************************************
> -
>       IoSetThreadHardErrorMode  (NTOSKRNL.EXE.@)
> */
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index ab952e528b..e08abb4150 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -913,7 +913,7 @@
>   @ stub PsJobType
>   @ stdcall PsLookupProcessByProcessId(ptr ptr)
>   @ stub PsLookupProcessThreadByCid
> -@ stub PsLookupThreadByThreadId
> +@ stdcall PsLookupThreadByThreadId(ptr ptr)
>   @ extern PsProcessType
>   @ stub PsReferenceImpersonationToken
>   @ stub PsReferencePrimaryToken
> diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c
> index c3839da3bf..75bf934445 100644
> --- a/dlls/ntoskrnl.exe/tests/driver.c
> +++ b/dlls/ntoskrnl.exe/tests/driver.c
> @@ -1167,6 +1167,22 @@ static void test_resource(void)
>       ok(status == STATUS_SUCCESS, "got status %#x\n", status);
>   }
>
> +static void test_lookup_thread(void)
> +{
> - NTSTATUS status;
> - PETHREAD thread = NULL;
> -
> - status = PsLookupThreadByThreadId(PsGetCurrentThreadId(), &thread);

This doesn't compile, you're missing #include "ddk/ntifs.h"
> - ok(!status, "PsLookupThreadByThreadId failed: %#x\n", status);
> - ok((PKTHREAD)thread == KeGetCurrentThread(), "thread != KeGetCurrentThread\n");
> -
> - if (thread)
> -
>    ObDereferenceObject(thread);
> -
> - status = PsLookupThreadByThreadId(NULL, &thread);
> - ok(status == STATUS_INVALID_PARAMETER, "PsLookupThreadByThreadId returned %#x\n", status);
>
> +}
> - static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *stack, ULONG_PTR *info)
> {
>     ULONG length = stack->Parameters.DeviceIoControl.OutputBufferLength;
>
> @@ -1210,6 +1226,7 @@ static NTSTATUS main_test(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *st
>       test_lookaside_list();
>       test_ob_reference(test_input->path);
>       test_resource();
> - test_lookup_thred();

And this won't build either.
>   /* print process report */
>   if (winetest_debug)
>
> diff --git a/include/ddk/ntifs.h b/include/ddk/ntifs.h
> index abe357fbc9..9b57ae7ad7 100644
> --- a/include/ddk/ntifs.h
> +++ b/include/ddk/ntifs.h
> @@ -131,6 +131,7 @@ typedef struct _FS_FILTER_CALLBACKS
>
> BOOLEAN WINAPI FsRtlIsNameInExpression(PUNICODE_STRING, PUNICODE_STRING, BOOLEAN, PWCH);
>   NTSTATUS WINAPI ObQueryNameString(PVOID,POBJECT_NAME_INFORMATION,ULONG,PULONG);
> +NTSTATUS WINAPI PsLookupThreadByThreadId(HANDLE,PETHREAD*);
>   void WINAPI PsRevertToSelf(void);
>
> #endif

Even with fixed test compilation, it will fail on Wine without patch 6.

Jacek