Re: [ros-dev] [ros-diffs] [dchapyshev] 72655: [NtUser] Try to fix tests for user32:EnumDisplaySettings

[Thomas Faber]

I've seen like 4 people change this stuff recently and not one of them
got it 100% right.
So here are the rules for how to use probes and SEH:
* Probe pointers before accessing them. This ensures you're not reading
  kernel mode memory (and does pretty much nothing else)
* Never access user mode pointers outside of SEH. The application
  controls its address space, so protection, physical memory mapping
  and contents can change out from under you at any point. For most
  purposes, MmProbeAndLockPages is the only way to get around this.
* Sometimes there is no alternative to copying data to kernel memory.
  If there is a pointer stored in user mode memory, you need a copy of
  it because you can't rely on it not changing after your probe (even
  if you lock down the memory).
* In case of an exception you usually want to return the exception code.
  There are exceptions (heh) to this rule, but if you think you've
  found one, better be sure and show it with a test.
* If you make a pool allocation whose size is directly influenced by
  user mode input, there's a good chance you'll want to use
  ExAllocatePoolWithQuotaTag

In particular this means:
* Probing something or accessing it once does not save you from using
  SEH every single time you access the user mode memory
* Probing UserModePointer->StructMemberPointer is never correct, you
  need to make a copy of the struct or that particular member, since
  the memory can change
* The ProbeForReadUnicodeString function is not useful unless you use
  its return value


Hope this helps. Thanks for hanging in there.
-T


On 2016-09-11 18:13, dchapyshev(a)svn.reactos.org wrote:
>   Author: dchapyshev
>  Date: Sun Sep 11 16:13:46 2016
>  New Revision: 72655
>  
>  URL: http://svn.reactos.org/svn/reactos?rev=72655&view=rev
>  Log:
>  [NtUser] Try to fix tests for user32:EnumDisplaySettings
>  
>  Modified:
>      trunk/reactos/win32ss/user/ntuser/display.c
>  
>  Modified: trunk/reactos/win32ss/user/ntuser/display.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/win32ss/user/ntuser/displa…
>  ==============================================================================
>  --- trunk/reactos/win32ss/user/ntuser/display.c	[iso-8859-1] (original)
>  +++ trunk/reactos/win32ss/user/ntuser/display.c	[iso-8859-1] Sun Sep 11 16:13:46 2016
>  @@ -443,7 +443,7 @@
>       {
>           /* No device found */
>           ERR("No PDEV found!\n");
>  -        return STATUS_UNSUCCESSFUL;
>  +        return STATUS_INVALID_PARAMETER_1;
>       }
>   
>       *ppdm = ppdev->pdmwDev;
>  @@ -474,7 +474,7 @@
>       {
>           /* No device found */
>           ERR("No device found!\n");
>  -        return STATUS_UNSUCCESSFUL;
>  +        return STATUS_INVALID_PARAMETER_1;
>       }
>   
>       iFoundMode = 0;
>  @@ -571,13 +571,18 @@
>   
>       _SEH2_TRY
>       {
>  -        ProbeForWrite(lpDevMode, sizeof(DEVMODEW), 1);
>  +        ProbeForRead(lpDevMode, sizeof(DEVMODEW), 1);
>  +
>  +        cbSize = lpDevMode->dmSize;
>  +        cbExtra = lpDevMode->dmDriverExtra;
>  +
>  +        ProbeForWrite(lpDevMode, cbSize + cbExtra, 1);
>       }
>       _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>       {
>           _SEH2_YIELD(return _SEH2_GetExceptionCode());
>       }
>  -    _SEH2_END
>  +    _SEH2_END;
>   
>       if (lpDevMode->dmSize != sizeof(DEVMODEW))
>       {
>  @@ -586,31 +591,30 @@
>   
>       if (pustrDevice)
>       {
>  -       if (pustrDevice->Buffer == NULL || pustrDevice->Length == 0)
>  -       {
>  -           Status = STATUS_INVALID_PARAMETER_1;
>  -       }
>  -
>           /* Initialize destination string */
>           RtlInitEmptyUnicodeString(&ustrDevice, awcDevice, sizeof(awcDevice));
>   
>           _SEH2_TRY
>           {
>               /* Probe the UNICODE_STRING and the buffer */
>  -            ProbeForRead(pustrDevice, sizeof(UNICODE_STRING), 1);
>  -            ProbeForRead(pustrDevice->Buffer, pustrDevice->Length, 1);
>  +            ProbeForReadUnicodeString(pustrDevice);
>  +
>  +            if (!pustrDevice->Length || !pustrDevice->Buffer)
>  +                ExRaiseStatus(STATUS_NO_MEMORY);
>  +
>  +            ProbeForRead(pustrDevice->Buffer, pustrDevice->Length,
> sizeof(UCHAR));
>   
>               /* Copy the string */
>               RtlCopyUnicodeString(&ustrDevice, pustrDevice);
>           }
>           _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
>           {
>  -            _SEH2_YIELD(return _SEH2_GetExceptionCode());
>  -        }
>  -        _SEH2_END
>  +            _SEH2_YIELD(return STATUS_INVALID_PARAMETER_1);
>  +        }
>  +        _SEH2_END;
>   
>           pustrDevice = &ustrDevice;
>  -   }
>  +    }
>   
>       /* Acquire global USER lock */
>       UserEnterShared();
>  @@ -642,11 +646,6 @@
>           /* Copy some information back */
>           _SEH2_TRY
>           {
>  -            ProbeForRead(lpDevMode, sizeof(DEVMODEW), 1);
>  -            cbSize = lpDevMode->dmSize;
>  -            cbExtra = lpDevMode->dmDriverExtra;
>  -
>  -            ProbeForWrite(lpDevMode, cbSize + cbExtra, 1);
>               /* Output what we got */
>               RtlCopyMemory(lpDevMode, pdm, min(cbSize, pdm->dmSize));
>   
>  @@ -663,13 +662,6 @@
>               Status = _SEH2_GetExceptionCode();
>           }
>           _SEH2_END;
>  -    }
>  -    else
>  -    {
>  -        if (Status == STATUS_UNSUCCESSFUL)
>  -        {
>  -            Status = STATUS_INVALID_PARAMETER_1;
>  -        }
>       }
>   
>       return Status;
>  
>