Re: [ros-dev] [ros-diffs] [rharabien] 51108: Fix ProbeForRead. It wasn't ever checking if memory can be accessed. Thanks to big-endian it wasn't breaking MmUserProbeAddress as well. Code is now nearly the same as in ProbeFo...

[Timo Kreuzer]

Windows doesn't do any access checks in ProbeForRead, it only checks the 
range and alignment. The MmUserProbeAddress access is used to raise an 
exception with the appropriate parameters. So the old version was 
correct (except for the misleading comment maybe)


Am 21.03.2011 15:43, schrieb rharabien(a)svn.reactos.org:
>   Author: rharabien
>  Date: Mon Mar 21 14:43:56 2011
>  New Revision: 51108
>
>  URL: http://svn.reactos.org/svn/reactos?rev=51108&view=rev
>  Log:
>  Fix ProbeForRead. It wasn't ever checking if memory can be accessed. Thanks to
> big-endian it wasn't breaking MmUserProbeAddress as well. Code is now nearly the same
> as in ProbeForWrite. It shouldn't break anything. If it does, it's not bug in this
> code. :)
>
>  Modified:
>       trunk/reactos/ntoskrnl/ex/exintrin.c
>
>  Modified: trunk/reactos/ntoskrnl/ex/exintrin.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/exintrin.c?rev…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/ex/exintrin.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/ex/exintrin.c [iso-8859-1] Mon Mar 21 14:43:56 2011
>  @@ -103,6 +103,8 @@
>                 IN SIZE_T Length,
>                 IN ULONG Alignment)
>    {
>  +	ULONG_PTR Last, Current = (ULONG_PTR)Address;
>  +	CHAR Temp;
>        PAGED_CODE();
>
>        /* Only probe if we have a valid length */
>  @@ -115,18 +117,31 @@
>                   (Alignment == 8) ||
>                   (Alignment == 16));
>
>  -        /* Check for correct alignment */
>  -        if (((ULONG_PTR)Address&  (Alignment - 1)) != 0)
>  +        /* Check the alignment */
>  +        if ((Current&  (Alignment - 1)) != 0)
>            {
>                /* Incorrect alignment */
>                ExRaiseDatatypeMisalignment();
>            }
>  -        else if (((ULONG_PTR)Address + Length)<  (ULONG_PTR)Address ||
>  -                 ((ULONG_PTR)Address + Length)>  (ULONG_PTR)MmUserProbeAddress)
>  +
>  +        /* Get the end address */
>  +        Last = Current + Length - 1;
>  +        if ((Last<  Current) || (Last>= (ULONG_PTR)MmUserProbeAddress))
>  +        {
>  +            /* Raise an access violation */
>  +            ExRaiseAccessViolation();
>  +        }
>  +
>  +        /* Round down to the last page */
>  +        Last = PAGE_ROUND_DOWN(Last) + PAGE_SIZE;
>  +        do
>            {
>                /* Attempt a read */
>  -            *(volatile CHAR* const)MmUserProbeAddress = 0;
>  -        }
>  +            Temp = *(volatile CHAR*)Current;
>  +
>  +            /* Go to the next address */
>  +            Current = PAGE_ROUND_DOWN(Current) + PAGE_SIZE;
>  +        } while (Current != Last);
>        }
>    }
>
>
>
>

[Minas Abrahamyan]

>   So the old version was correct (except for the
> misleading comment maybe) 
At the same time, with /FIRSTCHANCE key enabled
do we need ROS to popup kdbg at every start?
It's so bothering...
just annoying

On Tue, Mar 22, 2011 at 4:58 AM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:
>  
>  Windows doesn't do any access checks in ProbeForRead, it only checks the
>  range and alignment. The MmUserProbeAddress access is used to raise an
>  exception with the appropriate parameters. So the old version was correct
>  (except for the misleading comment maybe)
>
>
>  Am 21.03.2011 15:43, schrieb rharabien(a)svn.reactos.org:
> >  
> >  Author: rharabien
> >  Date: Mon Mar 21 14:43:56 2011
> >  New Revision: 51108
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=51108&view=rev
> >  Log:
> >  Fix ProbeForRead. It wasn't ever checking if memory can be accessed.
> >  Thanks to big-endian it wasn't breaking MmUserProbeAddress as well. Code is
> >  now nearly the same as in ProbeForWrite. It shouldn't break anything. If it
> >  does, it's not bug in this code. :)
> >
> >  Modified:
> >      trunk/reactos/ntoskrnl/ex/exintrin.c
> >
> >  Modified: trunk/reactos/ntoskrnl/ex/exintrin.c
> >  URL:
> >
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/exintrin.c?rev…
> >
> >  ==============================================================================
> >  --- trunk/reactos/ntoskrnl/ex/exintrin.c [iso-8859-1] (original)
> >  +++ trunk/reactos/ntoskrnl/ex/exintrin.c [iso-8859-1] Mon Mar 21 14:43:56
> >  2011
> >  @@ -103,6 +103,8 @@
> >                IN SIZE_T Length,
> >                IN ULONG Alignment)
> >   {
> >  +       ULONG_PTR Last, Current = (ULONG_PTR)Address;
> >  +       CHAR Temp;
> >       PAGED_CODE();
> >
> >       /* Only probe if we have a valid length */
> >  @@ -115,18 +117,31 @@
> >                  (Alignment == 8) ||
> >                  (Alignment == 16));
> >
> >  -        /* Check for correct alignment */
> >  -        if (((ULONG_PTR)Address&  (Alignment - 1)) != 0)
> >  +        /* Check the alignment */
> >  +        if ((Current&  (Alignment - 1)) != 0)
> >           {
> >               /* Incorrect alignment */
> >               ExRaiseDatatypeMisalignment();
> >           }
> >  -        else if (((ULONG_PTR)Address + Length)<  (ULONG_PTR)Address ||
> >  -                 ((ULONG_PTR)Address + Length)>
> >   (ULONG_PTR)MmUserProbeAddress)
> >  +
> >  +        /* Get the end address */
> >  +        Last = Current + Length - 1;
> >  +        if ((Last<  Current) || (Last>= (ULONG_PTR)MmUserProbeAddress))
> >  +        {
> >  +            /* Raise an access violation */
> >  +            ExRaiseAccessViolation();
> >  +        }
> >  +
> >  +        /* Round down to the last page */
> >  +        Last = PAGE_ROUND_DOWN(Last) + PAGE_SIZE;
> >  +        do
> >           {
> >               /* Attempt a read */
> >  -            *(volatile CHAR* const)MmUserProbeAddress = 0;
> >  -        }
> >  +            Temp = *(volatile CHAR*)Current;
> >  +
> >  +            /* Go to the next address */
> >  +            Current = PAGE_ROUND_DOWN(Current) + PAGE_SIZE;
> >  +        } while (Current != Last);
> >       }
> >   }
> >
> >
> >
> >  
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[Timo Kreuzer]

Am 22.03.2011 02:13, schrieb Minas Abrahamyan:
>   At the same time, with /FIRSTCHANCE key enabled
>  do we need ROS to popup kdbg at every start?
>  It's so bothering...
>  just annoying 
Is this being triggered by this commit? It would be interesting as it

might light up a possible bug somewhere.