Re: [ros-dev] [ros-diffs] [jimtabor] 38518: - Removed SEH abuse and add notes for the hook code, in NtUserDefSetText. - Update NtUserCallHwndLock subfunctions.

[Timo Kreuzer]

SEH is still needed. SafeText doesn't really deserve it's name, as it's
only a safe copy of the UNICODE_STRING structure  returned by
ProbeForReadUnicodeString(), but with the still unsafe string buffer.
Also the Buffer was never probed (ProbeForReadUnicodeString only checks
the UNICODE_STRING and copies it)

IMO the function is dangerous, as it implies that the Buffer was probed,
too.

Timo

[e-mail] schrieb:
> Author: jimtabor
> Date: Fri Jan  2 22:02:54 2009
> New Revision: 38518
>
> URL: http://svn.reactos.org/svn/reactos?rev=38518&view=rev
> Log:
> Removed SEH abuse and add notes for the hook code, in NtUserDefSetText.
> Update NtUserCallHwndLock subfunctions.
>
> Modified:
>     trunk/reactos/subsystems/win32/win32k/ntuser/simplecall.c
>     trunk/reactos/subsystems/win32/win32k/ntuser/window.c

...
> Modified: trunk/reactos/subsystems/win32/win32k/ntuser/window.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntu...
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/ntuser/window.c [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/ntuser/window.c [iso-8859-1] Fri Jan  2 22:02:54 2009
> @@ -4569,57 +4569,47 @@
>     }
>     Wnd = Window->Wnd;
> if(SafeText.Length != 0)
> {
>  _SEH2_TRY
>  {
>      if (Wnd->WindowName.MaximumLength > 0 &&
>          SafeText.Length <= Wnd->WindowName.MaximumLength - sizeof(UNICODE_NULL))
>      {
>          ASSERT(Wnd->WindowName.Buffer != NULL);
>          Wnd->WindowName.Length = SafeText.Length;
>          Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
>          RtlCopyMemory(Wnd->WindowName.Buffer,
>                        SafeText.Buffer,
>                        SafeText.Length);
>      }
>      else
>      {
>          PWCHAR buf;
>          Wnd->WindowName.MaximumLength = Wnd->WindowName.Length = 0;
>          buf = Wnd->WindowName.Buffer;
>          Wnd->WindowName.Buffer = NULL;
>          if (buf != NULL)
>          {
>              DesktopHeapFree(Wnd->pdesktop,
>                              buf);
>          }
>          Wnd->WindowName.Buffer = DesktopHeapAlloc(Wnd->pdesktop,
>                                                    SafeText.Length + sizeof(UNICODE_NULL));
>          if (Wnd->WindowName.Buffer != NULL)
>          {
>              Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
>              RtlCopyMemory(Wnd->WindowName.Buffer,
>                            SafeText.Buffer,
>                            SafeText.Length);
>              Wnd->WindowName.MaximumLength = SafeText.Length + sizeof(UNICODE_NULL);
>              Wnd->WindowName.Length = SafeText.Length;
>          }
>          else
>          {
>              SetLastWin32Error(ERROR_NOT_ENOUGH_MEMORY);
>              Ret = FALSE;
>          }
>      }
>  }
>  _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>  {
>       SetLastNtError(_SEH2_GetExceptionCode());
>       Ret = FALSE;
>  }
>  _SEH2_END;
> if (SafeText.Length != 0)
> {
>  if (Wnd->WindowName.MaximumLength > 0 &&
>      SafeText.Length <= Wnd->WindowName.MaximumLength - sizeof(UNICODE_NULL))
>  {
>     ASSERT(Wnd->WindowName.Buffer != NULL);
>     Wnd->WindowName.Length = SafeText.Length;
>     Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
>     RtlCopyMemory(Wnd->WindowName.Buffer,
>                          SafeText.Buffer,
>                          SafeText.Length);
>  }
>  else
>  {
>     PWCHAR buf;
>     Wnd->WindowName.MaximumLength = Wnd->WindowName.Length = 0;
>     buf = Wnd->WindowName.Buffer;
>     Wnd->WindowName.Buffer = NULL;
>     if (buf != NULL)
>     {
>        DesktopHeapFree(Wnd->pdesktop, buf);
>     }
>     Wnd->WindowName.Buffer = DesktopHeapAlloc(Wnd->pdesktop,
>                                               SafeText.Length + sizeof(UNICODE_NULL));
>     if (Wnd->WindowName.Buffer != NULL)
>     {
>        Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
>        RtlCopyMemory(Wnd->WindowName.Buffer,
>                             SafeText.Buffer,
>                             SafeText.Length);
>        Wnd->WindowName.MaximumLength = SafeText.Length + sizeof(UNICODE_NULL);
>        Wnd->WindowName.Length = SafeText.Length;
>     }
>     else
>     {
>        SetLastWin32Error(ERROR_NOT_ENOUGH_MEMORY);
>        Ret = FALSE;
>     }
>  }
> }
>   else
>   {
>
> @@ -4628,6 +4618,9 @@
>            Wnd->WindowName.Buffer[0] = L'\0';
>     }
> // HAX! FIXME! Windows does not do this in here!
> // In User32, these are called after: NotifyWinEvent EVENT_OBJECT_NAMECHANGE than
> // RepaintButton, StaticRepaint, NtUserCallHwndLock HWNDLOCK_ROUTINE_REDRAWFRAMEANDHOOK, etc.
> /* Send shell notifications */
> if (!IntGetOwner(Window) && !IntGetParent(Window))
> {

[Thomas Bluemel]

ProbeForReadUnicodeString should at least probe the buffers, otherwise 
the function is pointless. I believe at one point it did, and it was 
probably removed for some strange reason. The reason it copies the 
UNICODE_STRING is so that the pointers can't be modified anymore.

Thomas

Timo Kreuzer wrote:
> SEH is still needed. SafeText doesn't really deserve it's name, as it's
> only a safe copy of the UNICODE_STRING structure  returned by
> ProbeForReadUnicodeString(), but with the still unsafe string buffer.
> Also the Buffer was never probed (ProbeForReadUnicodeString only checks
> the UNICODE_STRING and copies it)
>
> IMO the function is dangerous, as it implies that the Buffer was probed,
> too.
>
> Timo
>
> [e-mail] schrieb:
> > Author: jimtabor
> > Date: Fri Jan  2 22:02:54 2009
> > New Revision: 38518
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=38518&view=rev
> > Log:
> > Removed SEH abuse and add notes for the hook code, in NtUserDefSetText.
> > Update NtUserCallHwndLock subfunctions.
> >
> > Modified:
> >     trunk/reactos/subsystems/win32/win32k/ntuser/simplecall.c
> >     trunk/reactos/subsystems/win32/win32k/ntuser/window.c
>
> ...
> > Modified: trunk/reactos/subsystems/win32/win32k/ntuser/window.c
> > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntu...
> > ==============================================================================
> > --- trunk/reactos/subsystems/win32/win32k/ntuser/window.c [iso-8859-1] (original)
> > +++ trunk/reactos/subsystems/win32/win32k/ntuser/window.c [iso-8859-1] Fri Jan  2 22:02:54 2009
> > @@ -4569,57 +4569,47 @@
> >     }
> >     Wnd = Window->Wnd;
> > if(SafeText.Length != 0)
> > {
> >  _SEH2_TRY
> >  {
> >      if (Wnd->WindowName.MaximumLength > 0 &&
> >          SafeText.Length <= Wnd->WindowName.MaximumLength - sizeof(UNICODE_NULL))
> >      {
> >          ASSERT(Wnd->WindowName.Buffer != NULL);
> >          Wnd->WindowName.Length = SafeText.Length;
> >          Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
> >          RtlCopyMemory(Wnd->WindowName.Buffer,
> >                        SafeText.Buffer,
> >                        SafeText.Length);
> >      }
> >      else
> >      {
> >          PWCHAR buf;
> >          Wnd->WindowName.MaximumLength = Wnd->WindowName.Length = 0;
> >          buf = Wnd->WindowName.Buffer;
> >          Wnd->WindowName.Buffer = NULL;
> >          if (buf != NULL)
> >          {
> >              DesktopHeapFree(Wnd->pdesktop,
> >                              buf);
> >          }
> >          Wnd->WindowName.Buffer = DesktopHeapAlloc(Wnd->pdesktop,
> >                                                    SafeText.Length + sizeof(UNICODE_NULL));
> >          if (Wnd->WindowName.Buffer != NULL)
> >          {
> >              Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
> >              RtlCopyMemory(Wnd->WindowName.Buffer,
> >                            SafeText.Buffer,
> >                            SafeText.Length);
> >              Wnd->WindowName.MaximumLength = SafeText.Length + sizeof(UNICODE_NULL);
> >              Wnd->WindowName.Length = SafeText.Length;
> >          }
> >          else
> >          {
> >              SetLastWin32Error(ERROR_NOT_ENOUGH_MEMORY);
> >              Ret = FALSE;
> >          }
> >      }
> >  }
> >  _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> >  {
> >       SetLastNtError(_SEH2_GetExceptionCode());
> >       Ret = FALSE;
> >  }
> >  _SEH2_END;
> > if (SafeText.Length != 0)
> > {
> >  if (Wnd->WindowName.MaximumLength > 0 &&
> >      SafeText.Length <= Wnd->WindowName.MaximumLength - sizeof(UNICODE_NULL))
> >  {
> >     ASSERT(Wnd->WindowName.Buffer != NULL);
> >     Wnd->WindowName.Length = SafeText.Length;
> >     Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
> >     RtlCopyMemory(Wnd->WindowName.Buffer,
> >                          SafeText.Buffer,
> >                          SafeText.Length);
> >  }
> >  else
> >  {
> >     PWCHAR buf;
> >     Wnd->WindowName.MaximumLength = Wnd->WindowName.Length = 0;
> >     buf = Wnd->WindowName.Buffer;
> >     Wnd->WindowName.Buffer = NULL;
> >     if (buf != NULL)
> >     {
> >        DesktopHeapFree(Wnd->pdesktop, buf);
> >     }
> >     Wnd->WindowName.Buffer = DesktopHeapAlloc(Wnd->pdesktop,
> >                                               SafeText.Length + sizeof(UNICODE_NULL));
> >     if (Wnd->WindowName.Buffer != NULL)
> >     {
> >        Wnd->WindowName.Buffer[SafeText.Length / sizeof(WCHAR)] = L'\0';
> >        RtlCopyMemory(Wnd->WindowName.Buffer,
> >                             SafeText.Buffer,
> >                             SafeText.Length);
> >        Wnd->WindowName.MaximumLength = SafeText.Length + sizeof(UNICODE_NULL);
> >        Wnd->WindowName.Length = SafeText.Length;
> >     }
> >     else
> >     {
> >        SetLastWin32Error(ERROR_NOT_ENOUGH_MEMORY);
> >        Ret = FALSE;
> >     }
> >  }
> > }
> >   else
> >   {
> >
> > @@ -4628,6 +4618,9 @@
> >            Wnd->WindowName.Buffer[0] = L'\0';
> >     }
> > // HAX! FIXME! Windows does not do this in here!
> > // In User32, these are called after: NotifyWinEvent EVENT_OBJECT_NAMECHANGE than
> > // RepaintButton, StaticRepaint, NtUserCallHwndLock HWNDLOCK_ROUTINE_REDRAWFRAMEANDHOOK, etc.
> > /* Send shell notifications */
> > if (!IntGetOwner(Window) && !IntGetParent(Window))
> > {
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[James Tabor]

Okay!

So, ProbeForReadUnicodeString is crippled and only copies the structure data...
I see,,,, I must add that we are using the wrong structure too.
LARGE_UNICODE_STRING is passed not that other one.

Thanks,
James

On Sat, Jan 3, 2009 at 10:18 AM, Thomas Bluemel [e-mail] wrote:
> ProbeForReadUnicodeString should at least probe the buffers, otherwise
> the function is pointless. I believe at one point it did, and it was
> probably removed for some strange reason. The reason it copies the
> UNICODE_STRING is so that the pointers can't be modified anymore.
>
> Thomas
>
> Timo Kreuzer wrote:
> > SEH is still needed. SafeText doesn't really deserve it's name, as it's
> > only a safe copy of the UNICODE_STRING structure  returned by
> > ProbeForReadUnicodeString(), but with the still unsafe string buffer.
> > Also the Buffer was never probed (ProbeForReadUnicodeString only checks
> > the UNICODE_STRING and copies it)
> >
> > IMO the function is dangerous, as it implies that the Buffer was probed,
> > too.
> >
> > Timo

Ref:
http://www.reactos.org/wiki/index.php/Techwiki/win32k/LARGE_UNICODE_STRING

[Timo Kreuzer]

Thomas Bluemel wrote:
> ProbeForReadUnicodeString should at least probe the buffers, otherwise 
> the function is pointless. I believe at one point it did, and it was

Well as pointless as ProbeForReadUlong etc. They are all inlined
versions of ProbeForRead + returning the probed data.
Maybe the names should be changed to ProbeAndReadUlong etc.

Btw, what about inlining the normal ProbeForRead() for internal use?
> probably removed for some strange reason. The reason it copies the 
> UNICODE_STRING is so that the pointers can't be modified anymore.

There's ProbeAndCaptureUnicodeString for that. The reason it was not
used here is probably that it would be a duplicate copy.