Re: [PATCH] kernel32: return copy of environment block inGetEnvironmentStringsW

[Dmitry Timoshkov]

"Peter Beutner" <p.beutner(a)gmx.net> wrote:
> Make behaviour consistent with GetEnvironmentStringsA.
> Prevents crash for confused applications which first call
> GetEnvironmentStringsW and then want to free it with FreeEnvironmentStringsA.

This patch is wrong: http://blogs.msdn.com/matt_pietrek/archive/2004/08/25/220330.aspx
Probably an exception handler should be added inside of RtlFreeHeap instead.

-- 
Dmitry.

[PATCH] kernel32: return copy of environment block inGetEnvironmentStringsW

[Peter Beutner]

Dmitry Timoshkov schrieb:
> "Peter Beutner" <p.beutner(a)gmx.net> wrote:
> > Make behaviour consistent with GetEnvironmentStringsA.
> > Prevents crash for confused applications which first call
> > GetEnvironmentStringsW and then want to free it with
> > FreeEnvironmentStringsA.
>
> This patch is wrong:
> http://blogs.msdn.com/matt_pietrek/archive/2004/08/25/220330.aspx
> thx for the info. Hadn't thought that they would implement this
> inconsistent behaviour between the A/W variants :/.
> Probably an exception handler should be added inside of RtlFreeHeap
> instead.
> hm, i vaguely remember reading here that it was prefered not to cover up
> crashes produced by passing invalid pointers, unless absolutely necessary.
> And here we can fix that nicely in some other way.
> See attached patch. Would that be acceptable as well?

[PATCH] kernel32: return copy of environment blockinGetEnvironmentStringsW

[Dmitry Timoshkov]

"Peter Beutner" <p.beutner(a)gmx.net> wrote:
> > Probably an exception handler should be added inside of RtlFreeHeap
> > instead.
> > hm, i vaguely remember reading here that it was prefered not to cover up
> > crashes produced by passing invalid pointers, unless absolutely necessary.

If the crash is caused by a NULL pointer then the fix is easy and doesn't
require using an exception handler, but it's not possible in this case.
> And here we can fix that nicely in some other way.
> See attached patch. Would that be acceptable as well?
>
> diff --git a/dlls/kernel32/environ.c b/dlls/kernel32/environ.c
> index 414ccbf..f7890cc 100644
> --- a/dlls/kernel32/environ.c
> +++ b/dlls/kernel32/environ.c
> @@ -148,6 +148,10 @@ LPWSTR WINAPI GetEnvironmentStringsW(void)
>  */
> BOOL WINAPI FreeEnvironmentStringsA( LPSTR ptr )
> {
> +    /* broken app passes ptr it got from GetEnvironmentStringsW */
> +    if(ptr == NtCurrentTeb()->Peb->ProcessParameters->Environment)
> +        return TRUE;
> +
>     return HeapFree( GetProcessHeap(), 0, ptr );
> }

Personally I think that adding an exception handler or probably just a more
strict consistency check inside of RtlFreeHeap should be better.

-- 
Dmitry.

[PATCH] kernel32: return copy of environment blockinGetEnvironmentStringsW

[Alexandre Julliard]

"Dmitry Timoshkov" <dmitry(a)codeweavers.com> writes:
> "Peter Beutner" <p.beutner(a)gmx.net> wrote:
> > @@ -148,6 +148,10 @@ LPWSTR WINAPI GetEnvironmentStringsW(void)
> >  */
> > BOOL WINAPI FreeEnvironmentStringsA( LPSTR ptr )
> > {
> > +    /* broken app passes ptr it got from GetEnvironmentStringsW */
> > +    if(ptr == NtCurrentTeb()->Peb->ProcessParameters->Environment)
> > +        return TRUE;
> > +
> >     return HeapFree( GetProcessHeap(), 0, ptr );
> > }
>
> Personally I think that adding an exception handler or probably just a more
> strict consistency check inside of RtlFreeHeap should be better.

An exception handler is probably too slow, but yes RtlFreeHeap should
have noticed that this block isn't part of the heap. Try something
like this:

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 80ddaf2..61b25db 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -395,7 +395,8 @@ static SUBHEAP *HEAP_FindSubHeap(
     while (sub)
     {
         if (((const char *)ptr >= (const char *)sub) &&
-            ((const char *)ptr < (const char *)sub + sub->size)) return (SUBHEAP*)sub;
+            ((const char *)ptr < (const char *)sub + sub->size - sizeof(ARENA_INUSE)))
+            return (SUBHEAP *)sub;
         sub = sub->next;
     }
     return NULL;
@@ -783,7 +784,7 @@ static ARENA_FREE *HEAP_FindFreeBlock( HEAP *heap, SIZE_T size,
  *
  * Check that the pointer is inside the range possible for arenas.
  */
-static BOOL HEAP_IsValidArenaPtr( const HEAP *heap, const void *ptr )
+static BOOL HEAP_IsValidArenaPtr( const HEAP *heap, const ARENA_FREE *ptr )
 {
     int i;
     const SUBHEAP *subheap = HEAP_FindSubHeap( heap, ptr );
@@ -1003,13 +1004,12 @@ static BOOL HEAP_IsRealArena( HEAP *heapPtr,   /* [in] ptr to the heap */
     if (!(flags & HEAP_NO_SERIALIZE))
         RtlEnterCriticalSection( &heapPtr->critSection );
 
-    if (block)
+    if (block)  /* only check this single memory block */
     {
-        /* Only check this single memory block */
+        const ARENA_INUSE *arena = (const ARENA_INUSE *)block - 1;
 
-        if (!(subheap = HEAP_FindSubHeap( heapPtr, block )) ||
-            ((const char *)block < (char *)subheap + subheap->headerSize
-                                   + sizeof(ARENA_INUSE)))
+        if (!(subheap = HEAP_FindSubHeap( heapPtr, arena )) ||
+            ((const char *)arena < (char *)subheap + subheap->headerSize))
         {
             if (quiet == NOISY)
                 ERR("Heap %p: block %p is not inside heap\n", heapPtr, block );
@@ -1017,7 +1017,7 @@ static BOOL HEAP_IsRealArena( HEAP *heapPtr,   /* [in] ptr to the heap */
                 WARN("Heap %p: block %p is not inside heap\n", heapPtr, block );
             ret = FALSE;
         } else
-            ret = HEAP_ValidateInUseArena( subheap, (const ARENA_INUSE *)block - 1, quiet );
+            ret = HEAP_ValidateInUseArena( subheap, arena, quiet );
 
         if (!(flags & HEAP_NO_SERIALIZE))
             RtlLeaveCriticalSection( &heapPtr->critSection );

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH] kernel32: return copy of environment blockinGetEnvironmentStringsW

[Peter Beutner]

Alexandre Julliard schrieb:
> "Dmitry Timoshkov" <dmitry(a)codeweavers.com> writes:
> > "Peter Beutner" <p.beutner(a)gmx.net> wrote:
> > > @@ -148,6 +148,10 @@ LPWSTR WINAPI GetEnvironmentStringsW(void)
> > >  */
> > > BOOL WINAPI FreeEnvironmentStringsA( LPSTR ptr )
> > > {
> > > +    /* broken app passes ptr it got from GetEnvironmentStringsW */
> > > +    if(ptr == NtCurrentTeb()->Peb->ProcessParameters->Environment)
> > > +        return TRUE;
> > > +
> > >     return HeapFree( GetProcessHeap(), 0, ptr );
> > > }
> > > Personally I think that adding an exception handler or probably just a more
> > > strict consistency check inside of RtlFreeHeap should be better.
>
> An exception handler is probably too slow, but yes RtlFreeHeap should
> have noticed that this block isn't part of the heap. Try something
> like this:
>
> diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
> index 80ddaf2..61b25db 100644
> --- a/dlls/ntdll/heap.c
> +++ b/dlls/ntdll/heap.c
> @@ -395,7 +395,8 @@ static SUBHEAP *HEAP_FindSubHeap(
>      while (sub)
>      {
>          if (((const char *)ptr >= (const char *)sub) &&
> -            ((const char *)ptr < (const char *)sub + sub->size)) return (SUBHEAP*)sub;
> +            ((const char *)ptr < (const char *)sub + sub->size - sizeof(ARENA_INUSE)))
> +            return (SUBHEAP *)sub;
>          sub = sub->next;
>      }
>      return NULL;
> @@ -783,7 +784,7 @@ static ARENA_FREE *HEAP_FindFreeBlock( HEAP *heap, SIZE_T size,
>   *
>   * Check that the pointer is inside the range possible for arenas.
>   */
> -static BOOL HEAP_IsValidArenaPtr( const HEAP *heap, const void *ptr )
> +static BOOL HEAP_IsValidArenaPtr( const HEAP *heap, const ARENA_FREE *ptr )
>  {
>      int i;
>      const SUBHEAP *subheap = HEAP_FindSubHeap( heap, ptr );
> @@ -1003,13 +1004,12 @@ static BOOL HEAP_IsRealArena( HEAP *heapPtr,   /* [in] ptr to the heap */
>      if (!(flags & HEAP_NO_SERIALIZE))
>          RtlEnterCriticalSection( &heapPtr->critSection );
>
> -    if (block)
> +    if (block)  /* only check this single memory block */
>      {
> -        /* Only check this single memory block */
> +        const ARENA_INUSE *arena = (const ARENA_INUSE *)block - 1;
>
> -        if (!(subheap = HEAP_FindSubHeap( heapPtr, block )) ||
> -            ((const char *)block < (char *)subheap + subheap->headerSize
> -                                   + sizeof(ARENA_INUSE)))
> +        if (!(subheap = HEAP_FindSubHeap( heapPtr, arena )) ||
> +            ((const char *)arena < (char *)subheap + subheap->headerSize))
>          {
>              if (quiet == NOISY)
>                  ERR("Heap %p: block %p is not inside heap\n", heapPtr, block );
> @@ -1017,7 +1017,7 @@ static BOOL HEAP_IsRealArena( HEAP *heapPtr,   /* [in] ptr to the heap */
>                  WARN("Heap %p: block %p is not inside heap\n", heapPtr, block );
>              ret = FALSE;
>          } else
> -            ret = HEAP_ValidateInUseArena( subheap, (const ARENA_INUSE *)block - 1, quiet );
> +            ret = HEAP_ValidateInUseArena( subheap, arena, quiet );
>
> if (!(flags & HEAP_NO_SERIALIZE))
>              RtlLeaveCriticalSection( &heapPtr->critSection );

Yes that works. It doesn't crash anymore. Thx for looking into this.