kernel/heap: Avoid heap corruption on invalid Parameter in GlobalFree(), with Test (RESEND)

[Detlef Riekenberg]

Changed MAGIC_DEAD in the Test after the Comment from Dimitry

While writing a testcase for bug #4742, i realized, that Windows detected an
invalid Handle for LocalFree()/GlobalFree(), but we corrupt our Heap sometimes:

warn:heap:HEAP_ValidateInUseArena Heap 0x7fcf0000: invalid in-use arena magic for 0x7fd459d8

The Spec:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/memory/base/globalfree.asp


Changelog:

kernel/heap: Avoid heap corruption on invalid Parameter in GlobalFree(), with Test



Similar Patches for GlobalFlags(), GlobalSize(), GlobalLock(), GlobalUnlock()
and GlobalRealloc() will follow to match msdn.


-- 
By by ... Detlef
-------------- next part --------------
diff --git a/dlls/kernel/heap.c b/dlls/kernel/heap.c
index 59a5e9f..55ed2fe 100644
--- a/dlls/kernel/heap.c
+++ b/dlls/kernel/heap.c
@@ -683,13 +683,19 @@ HGLOBAL WINAPI GlobalReAlloc(
  *
  * Free a global memory object.
  *
+ * PARAMS
+ *  hmem [I] Handle of the global memory object
+ *
  * RETURNS
- *      NULL: Success
- *      Handle: Failure
+ *  Success: NULL
+ *  Failure: The provided handle
+ *
+ * NOTES
+ *   When the Memory-Object is invalid, lasterror is set to ERROR_INVALID_HANDLE
+ *
  */
-HGLOBAL WINAPI GlobalFree(
-                 HGLOBAL hmem /* [in] Handle of global memory object */
-) {
+HGLOBAL WINAPI GlobalFree(HGLOBAL hmem)
+{
     PGLOBAL32_INTERN pintern;
     HGLOBAL hreturned;
 
@@ -707,6 +713,10 @@ HGLOBAL WINAPI GlobalFree(
 
             if(pintern->Magic==MAGIC_GLOBAL_USED)
             {
+                /* When we do not kill the Magic here and GlobalFree() is
+                     called twice for a handle,the Memory is released again
+                     and corrupt our heap (ValidateInUseArea) */
+                pintern->Magic = 0xdead;
 
                 /* WIN98 does not make this test. That is you can free a */
                 /* block you have not unlocked. Go figure!!              */
@@ -719,6 +729,12 @@ HGLOBAL WINAPI GlobalFree(
                 if(!HeapFree(GetProcessHeap(), 0, pintern))
                     hreturned=hmem;
             }
+            else
+            {
+                WARN("invalid handle %p (Magic: 0x%04x) (did you free the handle twice?)\n", hmem, pintern->Magic);
+                SetLastError(ERROR_INVALID_HANDLE);
+                hreturned = hmem;
+            }
         }
     }
     __EXCEPT_PAGE_FAULT
diff --git a/dlls/kernel/tests/heap.c b/dlls/kernel/tests/heap.c
index a6284a0..8c2b7fc 100644
--- a/dlls/kernel/tests/heap.c
+++ b/dlls/kernel/tests/heap.c
@@ -25,6 +25,8 @@
 #include "winbase.h"
 #include "wine/test.h"
 
+#define MAGIC_DEAD 0xdeadbeef
+
 static SIZE_T resize_9x(SIZE_T size)
 {
     DWORD dwSizeAligned = (size + 3) & ~3;
@@ -35,7 +37,8 @@ START_TEST(heap)
 {
     void *mem;
     HGLOBAL gbl;
-    SIZE_T size;
+    HGLOBAL hsecond;
+    SIZE_T  size;
 
     /* Heap*() functions */
     mem = HeapAlloc(GetProcessHeap(), 0, 0);
@@ -75,6 +78,15 @@ START_TEST(heap)
     gbl = GlobalReAlloc(0, 10, GMEM_MOVEABLE);
     ok(gbl == NULL, "global realloc allocated memory\n");
 
+    /* invalid handles are catched in windows */
+    gbl = GlobalAlloc(GMEM_MOVEABLE, 256);
+    GlobalFree(gbl);
+    SetLastError(MAGIC_DEAD);
+    hsecond = GlobalFree(gbl);      /* invalid handle: free memory twice */
+    ok( (hsecond == gbl) && (GetLastError() == ERROR_INVALID_HANDLE),
+        "returned %p with 0x%08lx (expected %p with ERROR_INVALID_HANDLE)\n",
+        hsecond, GetLastError(), gbl);
+
     /* Local*() functions */
     gbl = LocalAlloc(LMEM_MOVEABLE, 0);
     ok(gbl != NULL, "local memory not allocated for size 0\n");
@@ -96,6 +108,15 @@ START_TEST(heap)
     gbl = LocalReAlloc(0, 10, LMEM_MOVEABLE);
     ok(gbl == NULL, "local realloc allocated memory\n");
 
+    /* invalid handles are catched in windows */
+    gbl = LocalAlloc(GMEM_MOVEABLE, 256);
+    LocalFree(gbl);
+    SetLastError(MAGIC_DEAD);
+    hsecond = LocalFree(gbl);       /* invalid handle: free memory twice */
+    ok( (hsecond == gbl) && (GetLastError() == ERROR_INVALID_HANDLE),
+        "returned %p with 0x%08lx (expected %p with ERROR_INVALID_HANDLE)\n",
+        hsecond, GetLastError(), gbl);
+
     /* trying to lock empty memory should give an error */
     gbl = GlobalAlloc(GMEM_MOVEABLE|GMEM_ZEROINIT,0);
     ok(gbl != NULL, "returned NULL\n");