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.

2018-12-10 Thread hermes . belusca
Hello Timo,

Let's make a deal: while I fix this in win32k, you may try to find a way to fix 
that one too in the SAC driver line 1192:

https://git.reactos.org/?p=reactos.git;a=blob;f=drivers/sac/driver/util.c;hb=d0c8636d1b41e07f67546c7f4f07df6ddbedb2a1#l1167

I used the object macros because I didn't want to have the (huge) overhead of 
calling ObQueryNameString() together with allocating memory buffers just to 
retrieve the necessary names that already exist somewhere. (Also we note that 
WinStation objects, by their nature, really wrap closely around the NT objects 
and that's why I didn't see any inconvenience in using the helper macros for 
retrieving said information).

Best,
Hermes‌

De : "Timo 
Kreuzer"
A : ros-dev@reactos.org,"Hermès Bélusca-Maïto"
Envoyé: lundi 10 décembre 2018 01:44
Objet : Re: [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.


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=43e2ab208a2d3d50b12b4689347f57ca83568dd9

 commit 43e2ab208a2d3d50b12b4689347f57ca83568dd9
 Author: Hermès Bélusca-Maïto
 AuthorDate: Sun Jun 17 19:40:32 2018 +0200
 Commit: Hermès Bélusca-Maïto
 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);

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.

2018-12-09 Thread 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=43e2ab208a2d3d50b12b4689347f57ca83568dd9

commit 43e2ab208a2d3d50b12b4689347f57ca83568dd9
Author: Hermès Bélusca-Maïto 
AuthorDate: Sun Jun 17 19:40:32 2018 +0200
Commit: Hermès Bélusca-Maïto 
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", ()->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(>Name);

-
  return STATUS_SUCCESS;
  }
  
@@ -449,8 +447,6 @@ IntCreateWindowStation(

  RtlZeroMemory(WindowStationObject, sizeof(WINSTATION_OBJECT));
  
  InitializeListHead(>DesktopListHead);

-WindowStationObject->Name = *ObjectAttributes->ObjectName;
-ObjectAttributes->ObjectName = NULL; // FIXME! (see 
NtUserCreateWindowStation())
  WindowStationObject->dwSessionId = NtCurrentPeb()->SessionId;
  Status = RtlCreateAtomTable(37, >AtomTable);
  if (!NT_SUCCESS(Status))
@@ -491,7 +487,7 @@ IntCreateWindowStation(
  }
  
  TRACE("IntCreateWindowStation created object 0x%p with name %wZ handle 0x%p\n",

-  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(, ServiceWinStaName);
  LocalObjectAttributes.ObjectName = 
  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