Re: [ros-dev] [ros-diffs] 07/18: [WIN32K:NTUSER] Get rid of the cached window station Name member, and instead just use the name stored in the NT Object's header. CORE-11933 and PR #621.

[Timo Kreuzer]

Why did you remove an abstraction and create additional dependencies on 
internal implementation details?

Win32k is not supposed to directly access internal kernel structures! 
The headers and macros shouldn't even be in NDK.

Please revert or fix this. And while you are at it, put an "#ifdef 
_NTOSKRNL_" around the stuff in NDK to prevent people from using it.

Thanks,
Timo


Am 19.08.2018 um 22:16 schrieb Hermès Bélusca-Maïto:
>  
> https://git.reactos.org/?p=reactos.git;a=commitdiff;h=43e2ab208a2d3d50b12b4…
>
>  commit 43e2ab208a2d3d50b12b4689347f57ca83568dd9
>  Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
>  AuthorDate: Sun Jun 17 19:40:32 2018 +0200
>  Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
>  CommitDate: Sun Aug 19 22:18:32 2018 +0200
>
>       [WIN32K:NTUSER] Get rid of the cached window station Name member, and instead just
> use the name stored in the NT Object's header.
>       CORE-11933 and PR #621.
>       
>       - Remove the related hack-FIXMEs;
>       - Adjust NtUserGetObjectInformation() in accordance.
>       - Retrieve the window-station/desktop object type string in
> NtUserGetObjectInformation()
>         also from the NT Object's header.
>       
>       Also simplify the UOI_FLAGS case of NtUserGetObjectInformation() by reading
>       the handle inheritance information directly from the OBJECT_HANDLE_INFORMATION
>       structure returned by ObReferenceObjectByHandle().
>  ---
>    win32ss/user/ntuser/sysparams.c |   3 +-
>    win32ss/user/ntuser/winsta.c    | 106 +++++++++++++++++++---------------------
>    win32ss/user/ntuser/winsta.h    |   1 -
>    3 files changed, 51 insertions(+), 59 deletions(-)
>
>  diff --git a/win32ss/user/ntuser/sysparams.c b/win32ss/user/ntuser/sysparams.c
>  index 7eedc028de..d0badba00e 100644
>  --- a/win32ss/user/ntuser/sysparams.c
>  +++ b/win32ss/user/ntuser/sysparams.c
>  @@ -33,7 +33,8 @@ BOOL g_PaintDesktopVersion = FALSE;
>            } \
>            else \
>            { \
>  -            ERR("NtUserSystemParametersInfo requires interactive window station
> (current is %wZ)\n", &GetW32ProcessInfo()->prpwinsta->Name); \
>  +            ERR("NtUserSystemParametersInfo requires interactive window station
> (current is %wZ)\n", \
>  +               
> &(OBJECT_HEADER_TO_NAME_INFO(OBJECT_TO_OBJECT_HEADER(GetW32ProcessInfo()->prpwinsta))->Name));
> \
>            } \
>            EngSetLastError(err); \
>            return 0; \
>  diff --git a/win32ss/user/ntuser/winsta.c b/win32ss/user/ntuser/winsta.c
>  index f373b1cedf..ba1b1eb57d 100644
>  --- a/win32ss/user/ntuser/winsta.c
>  +++ b/win32ss/user/ntuser/winsta.c
>  @@ -114,8 +114,6 @@ IntWinStaObjectDelete(
>    
>        RtlDestroyAtomTable(WinSta->AtomTable);
>    
>  -    RtlFreeUnicodeString(&WinSta->Name);
>  -
>        return STATUS_SUCCESS;
>    }
>    
>  @@ -449,8 +447,6 @@ IntCreateWindowStation(
>        RtlZeroMemory(WindowStationObject, sizeof(WINSTATION_OBJECT));
>    
>        InitializeListHead(&WindowStationObject->DesktopListHead);
>  -    WindowStationObject->Name = *ObjectAttributes->ObjectName;
>  -    ObjectAttributes->ObjectName = NULL; // FIXME! (see NtUserCreateWindowStation())
>        WindowStationObject->dwSessionId = NtCurrentPeb()->SessionId;
>        Status = RtlCreateAtomTable(37, &WindowStationObject->AtomTable);
>        if (!NT_SUCCESS(Status))
>  @@ -491,7 +487,7 @@ IntCreateWindowStation(
>        }
>    
>        TRACE("IntCreateWindowStation created object 0x%p with name %wZ handle
> 0x%p\n",
>  -          WindowStationObject, &WindowStationObject->Name, WindowStation);
>  +          WindowStationObject, ObjectAttributes->ObjectName, WindowStation);
>    
>        *phWinSta = WindowStation;
>        return STATUS_SUCCESS;
>  @@ -582,23 +578,7 @@ NtUserCreateWindowStation(
>                return NULL;
>            }
>    
>  -        WindowStationName.Length = wcslen(ServiceWinStaName) * sizeof(WCHAR);
>  -        WindowStationName.MaximumLength =
>  -            WindowStationName.Length + sizeof(UNICODE_NULL);
>  -        WindowStationName.Buffer =
>  -            ExAllocatePoolWithTag(PagedPool,
>  -                                  WindowStationName.MaximumLength,
>  -                                  TAG_STRING);
>  -        if (!WindowStationName.Buffer)
>  -        {
>  -            Status = STATUS_NO_MEMORY;
>  -            ERR("Impossible to build a valid window station name, Status
> 0x%08lx\n", Status);
>  -            SetLastNtError(Status);
>  -            return NULL;
>  -        }
>  -        RtlStringCbCopyW(WindowStationName.Buffer,
>  -                         WindowStationName.MaximumLength,
>  -                         ServiceWinStaName);
>  +        RtlInitUnicodeString(&WindowStationName, ServiceWinStaName);
>            LocalObjectAttributes.ObjectName = &WindowStationName;
>            AccessMode = KernelMode;
>        }
>  @@ -615,12 +595,7 @@ NtUserCreateWindowStation(
>                                        Unknown5,
>                                        Unknown6);
>    
>  -    // FIXME! Because in some situations we store the allocated window station name
>  -    // inside the window station, we must not free it now! We know this fact when
>  -    // IntCreateWindowStation() sets LocalObjectAttributes.ObjectName to NULL.
>  -    // This hack must be removed once we just use the stored Ob name instead
>  -    // (in which case we will always free the allocated name here).
>  -    if (LocalObjectAttributes.ObjectName)
>  +    if ((AccessMode == UserMode) && LocalObjectAttributes.ObjectName)
>            ExFreePoolWithTag(LocalObjectAttributes.ObjectName->Buffer, TAG_STRING);
>    
>        if (NT_SUCCESS(Status))
>  @@ -802,7 +777,11 @@ NtUserGetObjectInformation(
>        NTSTATUS Status;
>        PWINSTATION_OBJECT WinStaObject = NULL;
>        PDESKTOP DesktopObject = NULL;
>  +    POBJECT_HEADER ObjectHeader;
>  +    POBJECT_HEADER_NAME_INFO NameInfo;
>  +    OBJECT_HANDLE_INFORMATION HandleInfo;
>        USEROBJECTFLAGS ObjectFlags;
>  +    PUNICODE_STRING pStrNameU = NULL;
>        PVOID pvData = NULL;
>        SIZE_T nDataSize = 0;
>    
>  @@ -820,13 +799,13 @@ NtUserGetObjectInformation(
>        _SEH2_END;
>    
>        /* Try window station */
>  -    TRACE("Trying to open window station %p\n", hObject);
>  +    TRACE("Trying to open window station 0x%p\n", hObject);
>        Status = ObReferenceObjectByHandle(hObject,
>                                           0,
>                                           ExWindowStationObjectType,
>                                           UserMode,
>                                           (PVOID*)&WinStaObject,
>  -                                       NULL);
>  +                                       &HandleInfo);
>    
>        if (Status == STATUS_OBJECT_TYPE_MISMATCH)
>        {
>  @@ -852,23 +831,8 @@ NtUserGetObjectInformation(
>        {
>            case UOI_FLAGS:
>            {
>  -            OBJECT_HANDLE_ATTRIBUTE_INFORMATION HandleInfo;
>  -            ULONG BytesWritten;
>  -
>                ObjectFlags.fReserved = FALSE;
>  -
>  -            /* Check whether this handle is inheritable */
>  -            Status = ZwQueryObject(hObject,
>  -                                   ObjectHandleFlagInformation,
>  -                                   &HandleInfo,
>  -                                   sizeof(OBJECT_HANDLE_ATTRIBUTE_INFORMATION),
>  -                                   &BytesWritten);
>  -            if (!NT_SUCCESS(Status))
>  -            {
>  -                ERR("ZwQueryObject failed, Status 0x%08lx\n", Status);
>  -                break;
>  -            }
>  -            ObjectFlags.fInherit = HandleInfo.Inherit;
>  +            ObjectFlags.fInherit = !!(HandleInfo.HandleAttributes & OBJ_INHERIT);
>    
>                ObjectFlags.dwFlags = 0;
>                if (WinStaObject != NULL)
>  @@ -893,11 +857,24 @@ NtUserGetObjectInformation(
>    
>            case UOI_NAME:
>            {
>  -            // FIXME: Use either ObQueryNameString() or read directly that name inside
> the Object section!
>                if (WinStaObject != NULL)
>                {
>  -                pvData = WinStaObject->Name.Buffer;
>  -                nDataSize = WinStaObject->Name.Length + sizeof(WCHAR);
>  +                ObjectHeader = OBJECT_TO_OBJECT_HEADER(WinStaObject);
>  +                NameInfo = OBJECT_HEADER_TO_NAME_INFO(ObjectHeader);
>  +
>  +                if (NameInfo && (NameInfo->Name.Length > 0))
>  +                {
>  +                    /* Named window station */
>  +                    pStrNameU = &NameInfo->Name;
>  +                    nDataSize = pStrNameU->Length + sizeof(UNICODE_NULL);
>  +                }
>  +                else
>  +                {
>  +                    /* Unnamed window station (should never happen!) */
>  +                    ASSERT(FALSE);
>  +                    pStrNameU = NULL;
>  +                    nDataSize = sizeof(UNICODE_NULL);
>  +                }
>                    Status = STATUS_SUCCESS;
>                }
>                else if (DesktopObject != NULL)
>  @@ -917,14 +894,16 @@ NtUserGetObjectInformation(
>            {
>                if (WinStaObject != NULL)
>                {
>  -                pvData = L"WindowStation";
>  -                nDataSize = sizeof(L"WindowStation");
>  +                ObjectHeader = OBJECT_TO_OBJECT_HEADER(WinStaObject);
>  +                pStrNameU = &ObjectHeader->Type->Name;
>  +                nDataSize = pStrNameU->Length + sizeof(UNICODE_NULL);
>                    Status = STATUS_SUCCESS;
>                }
>                else if (DesktopObject != NULL)
>                {
>  -                pvData = L"Desktop";
>  -                nDataSize = sizeof(L"Desktop");
>  +                ObjectHeader = OBJECT_TO_OBJECT_HEADER(DesktopObject);
>  +                pStrNameU = &ObjectHeader->Type->Name;
>  +                nDataSize = pStrNameU->Length + sizeof(UNICODE_NULL);
>                    Status = STATUS_SUCCESS;
>                }
>                else
>  @@ -954,10 +933,25 @@ Exit:
>                *nLengthNeeded = nDataSize;
>    
>            /* Try to copy data to caller */
>  -        if (Status == STATUS_SUCCESS)
>  +        if (Status == STATUS_SUCCESS && (nDataSize > 0))
>            {
>                TRACE("Trying to copy data to caller (len = %lu, len needed =
> %lu)\n", nLength, nDataSize);
>  -            RtlCopyMemory(pvInformation, pvData, nDataSize);
>  +            if (pvData)
>  +            {
>  +                /* Copy the data */
>  +                RtlCopyMemory(pvInformation, pvData, nDataSize);
>  +            }
>  +            else if (pStrNameU)
>  +            {
>  +                /* Copy and NULL-terminate the string */
>  +                RtlCopyMemory(pvInformation, pStrNameU->Buffer,
> pStrNameU->Length);
>  +                ((PWCHAR)pvInformation)[pStrNameU->Length / sizeof(WCHAR)] =
> UNICODE_NULL;
>  +            }
>  +            else
>  +            {
>  +                /* Zero the memory */
>  +                RtlZeroMemory(pvInformation, nDataSize);
>  +            }
>            }
>        }
>        _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>  @@ -1026,8 +1020,6 @@ NtUserSetObjectInformation(
>    }
>    
>    
>  -
>  -
>    HWINSTA FASTCALL
>    UserGetProcessWindowStation(VOID)
>    {
>  diff --git a/win32ss/user/ntuser/winsta.h b/win32ss/user/ntuser/winsta.h
>  index 085f3bcb26..19b1479ec0 100644
>  --- a/win32ss/user/ntuser/winsta.h
>  +++ b/win32ss/user/ntuser/winsta.h
>  @@ -15,7 +15,6 @@ typedef struct _WINSTATION_OBJECT
>    {
>        DWORD dwSessionId;
>    
>  -    UNICODE_STRING Name;
>        LIST_ENTRY DesktopListHead;
>        PRTL_ATOM_TABLE AtomTable;
>        HANDLE ShellWindow;
>
>