[ros-diffs] [ion] 52800: [KERNEL32]: Bug #35: TlsSetValue should allocate the TlsExpansionSlots under the PEB Lock. Bug #36: TlsGetValue needs to set the last error to ERROR_SUCCESS when the index is valid (am...

Author: ion
Date: Sat Jul 23 11:28:35 2011
New Revision: 52800

URL: http://svn.reactos.org/svn/reactos?rev=52800&view=rev
Log:
[KERNEL32]:
Bug #35: TlsSetValue should allocate the TlsExpansionSlots under the PEB Lock.
Bug #36: TlsGetValue needs to set the last error to ERROR_SUCCESS when the index is valid
(amd even if no expansion slots exist!).
Bug #37: TlsFree did not check the return of NtSetInformationThread(ThreadZeroTlsCell).
Bug #38: TlsAlloc was setting error to ERROR_NO_MORE_ITEMS instead of STATUS_NO_MEMORY. In
principle the former is more accurate, but that's not how API compatibility works.
Optimize TLS functions not to call NtQueryTeb/Peb all the time. Cache the TEB/PEB value
instead.


Modified:
    trunk/reactos/dll/win32/kernel32/client/synch.c
    trunk/reactos/dll/win32/kernel32/client/thread.c

Modified: trunk/reactos/dll/win32/kernel32/client/synch.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/synch.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/synch.c [iso-8859-1] Sat Jul 23 11:28:35 2011
@@ -39,7 +39,7 @@
     LARGE_INTEGER Time;
     NTSTATUS Status;
     RTL_CALLER_ALLOCATED_ACTIVATION_CONTEXT_STACK_FRAME ActCtx;
-    
+
     /* APCs must execute with the default activation context */
     if (bAlertable)
     {
@@ -75,7 +75,7 @@
             Status = WAIT_FAILED;
         }
     } while ((Status == STATUS_ALERTED) && (bAlertable));
-    
+
     /* Cleanup the activation context */
     if (bAlertable) RtlDeactivateActivationContextUnsafeFast(&ActCtx);
 
@@ -119,7 +119,7 @@
     DWORD i;
     NTSTATUS Status;
     RTL_CALLER_ALLOCATED_ACTIVATION_CONTEXT_STACK_FRAME ActCtx;
-    
+
     /* APCs must execute with the default activation context */
     if (bAlertable)
     {
@@ -196,7 +196,7 @@
 
     /* Cleanup the activation context */
     if (bAlertable) RtlDeactivateActivationContextUnsafeFast(&ActCtx);
-    
+
     /* Return wait status */
     return Status;
 }
@@ -215,7 +215,7 @@
     LARGE_INTEGER Time;
     NTSTATUS Status;
     RTL_CALLER_ALLOCATED_ACTIVATION_CONTEXT_STACK_FRAME ActCtx;
-    
+
     /* APCs must execute with the default activation context */
     if (bAlertable)
     {
@@ -225,7 +225,7 @@
         ActCtx.Format =
RTL_CALLER_ALLOCATED_ACTIVATION_CONTEXT_STACK_FRAME_FORMAT_WHISTLER;
         RtlActivateActivationContextUnsafeFast(&ActCtx, NULL);
     }
-    
+
     /* Get real handle */
     hObjectToWaitOn = TranslateStdHandle(hObjectToWaitOn);
 
@@ -258,7 +258,7 @@
 
     /* Cleanup the activation context */
     if (bAlertable) RtlDeactivateActivationContextUnsafeFast(&ActCtx);
-    
+
     /* Return wait status */
     return Status;
 }
@@ -674,7 +674,7 @@
     PLARGE_INTEGER TimePtr;
     NTSTATUS errCode;
     RTL_CALLER_ALLOCATED_ACTIVATION_CONTEXT_STACK_FRAME ActCtx;
-    
+
     /* APCs must execute with the default activation context */
     if (bAlertable)
     {
@@ -702,7 +702,7 @@
         errCode = NtDelayExecution((BOOLEAN)bAlertable, TimePtr);
     }
     while ((bAlertable) && (errCode == STATUS_ALERTED));
-    
+
     /* Return the correct code */
     return (errCode == STATUS_USER_APC) ? WAIT_IO_COMPLETION : 0;
 }

Modified: trunk/reactos/dll/win32/kernel32/client/thread.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/thread.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/thread.c [iso-8859-1] Sat Jul 23 11:28:35
2011
@@ -230,7 +230,7 @@
     {
         ASSERT(FALSE);
     }
-    
+
     /* Success */
     if(lpThreadId) *lpThreadId = HandleToUlong(ClientId.UniqueThread);
 
@@ -939,51 +939,61 @@
 TlsAlloc(VOID)
 {
     ULONG Index;
-
+    PTEB Teb;
+    PPEB Peb;
+
+    /* Get the PEB and TEB, lock the PEB */
+    Teb = NtCurrentTeb();
+    Peb = Teb->ProcessEnvironmentBlock;
     RtlAcquirePebLock();
 
-    /* Try to get regular TEB slot. */
-    Index = RtlFindClearBitsAndSet(NtCurrentPeb()->TlsBitmap, 1, 0);
-    if (Index == ~0U)
-    {
-        /* If it fails, try to find expansion TEB slot. */
-        Index = RtlFindClearBitsAndSet(NtCurrentPeb()->TlsExpansionBitmap, 1, 0);
-        if (Index != ~0U)
+    /* Try to get regular TEB slot */
+    Index = RtlFindClearBitsAndSet(Peb->TlsBitmap, 1, 0);
+    if (Index != 0xFFFFFFFF)
+    {
+        /* Clear the value. */
+        Teb->TlsSlots[Index] = 0;
+        RtlReleasePebLock();
+        return Index;
+    }
+
+    /* If it fails, try to find expansion TEB slot. */
+    Index = RtlFindClearBitsAndSet(Peb->TlsExpansionBitmap, 1, 0);
+    if (Index != 0xFFFFFFFF)
+    {
+        /* Is there no expansion slot yet? */
+        if (!Teb->TlsExpansionSlots)
         {
-            if (NtCurrentTeb()->TlsExpansionSlots == NULL)
-            {
-                NtCurrentTeb()->TlsExpansionSlots = HeapAlloc(GetProcessHeap(),
-                                                              HEAP_ZERO_MEMORY,
-                                                              TLS_EXPANSION_SLOTS *
-                                                              sizeof(PVOID));
-            }
-
-            if (NtCurrentTeb()->TlsExpansionSlots == NULL)
-            {
-                RtlClearBits(NtCurrentPeb()->TlsExpansionBitmap, Index, 1);
-                Index = ~0;
-                SetLastError(ERROR_NOT_ENOUGH_MEMORY);
-            }
-            else
-            {
-                /* Clear the value. */
-                NtCurrentTeb()->TlsExpansionSlots[Index] = 0;
-                Index += TLS_MINIMUM_AVAILABLE;
-            }
+            /* Allocate an array */
+            Teb->TlsExpansionSlots = RtlAllocateHeap(RtlGetProcessHeap(),
+                                                     HEAP_ZERO_MEMORY,
+                                                     TLS_EXPANSION_SLOTS *
+                                                     sizeof(PVOID));
+        }
+
+        /* Did we get an array? */
+        if (!Teb->TlsExpansionSlots)
+        {
+            /* Fail */
+            RtlClearBits(Peb->TlsExpansionBitmap, Index, 1);
+            Index = 0xFFFFFFFF;
+            SetLastErrorByStatus(STATUS_NO_MEMORY);
         }
         else
         {
-            SetLastError(ERROR_NO_MORE_ITEMS);
+            /* Clear the value. */
+            Teb->TlsExpansionSlots[Index] = 0;
+            Index += TLS_MINIMUM_AVAILABLE;
         }
     }
     else
     {
-        /* Clear the value. */
-        NtCurrentTeb()->TlsSlots[Index] = 0;
-    }
-
+        /* Fail */
+        SetLastErrorByStatus(STATUS_NO_MEMORY);
+    }
+
+    /* Release the lock and return */
     RtlReleasePebLock();
-
     return Index;
 }
 
@@ -992,52 +1002,70 @@
  */
 BOOL
 WINAPI
-TlsFree(DWORD Index)
+TlsFree(IN DWORD Index)
 {
     BOOL BitSet;
-
-    if (Index >= TLS_EXPANSION_SLOTS + TLS_MINIMUM_AVAILABLE)
-    {
+    PPEB Peb;
+    ULONG TlsIndex;
+    PVOID TlsBitmap;
+    NTSTATUS Status;
+
+    /* Acquire the PEB lock and grab the PEB */
+    Peb = NtCurrentPeb();
+    RtlAcquirePebLock();
+
+    /* Check if the index is too high */
+    if (Index >= TLS_MINIMUM_AVAILABLE)
+    {
+        /* Check if it can fit in the expansion slots */
+        TlsIndex = Index - TLS_MINIMUM_AVAILABLE;
+        if (TlsIndex >= TLS_EXPANSION_SLOTS)
+        {
+            /* It's invalid */
+            SetLastErrorByStatus(STATUS_INVALID_PARAMETER);
+            RtlReleasePebLock();
+            return FALSE;
+        }
+        else
+        {
+            /* Use the expansion bitmap */
+            TlsBitmap = Peb->TlsExpansionBitmap;
+            Index = TlsIndex;
+        }
+    }
+    else
+    {
+        /* Use the normal bitmap */
+        TlsBitmap = Peb->TlsBitmap;
+    }
+
+    /* Check if the index was set */
+    BitSet = RtlAreBitsSet(TlsBitmap, Index, 1);
+    if (BitSet)
+    {
+        /* Tell the kernel to free the TLS cells */
+        Status = NtSetInformationThread(NtCurrentThread(),
+                                        ThreadZeroTlsCell,
+                                        &Index,
+                                        sizeof(DWORD));
+        if (!NT_SUCCESS(Status))
+        {
+            SetLastErrorByStatus(STATUS_INVALID_PARAMETER);
+            return FALSE;
+        }
+
+        /* Clear the bit */
+        RtlClearBits(TlsBitmap, Index, 1);
+    }
+    else
+    {
+        /* Fail */
         SetLastErrorByStatus(STATUS_INVALID_PARAMETER);
         return FALSE;
     }
 
-    RtlAcquirePebLock();
-
-    if (Index >= TLS_MINIMUM_AVAILABLE)
-    {
-        BitSet = RtlAreBitsSet(NtCurrentPeb()->TlsExpansionBitmap,
-                               Index - TLS_MINIMUM_AVAILABLE,
-                               1);
-
-       if (BitSet)
-           RtlClearBits(NtCurrentPeb()->TlsExpansionBitmap,
-                        Index - TLS_MINIMUM_AVAILABLE,
-                        1);
-    }
-    else
-    {
-        BitSet = RtlAreBitsSet(NtCurrentPeb()->TlsBitmap, Index, 1);
-        if (BitSet)
-            RtlClearBits(NtCurrentPeb()->TlsBitmap, Index, 1);
-    }
-
-    if (BitSet)
-    {
-        /* Clear the TLS cells (slots) in all threads of the current process. */
-        NtSetInformationThread(NtCurrentThread(),
-                               ThreadZeroTlsCell,
-                               &Index,
-                               sizeof(DWORD));
-    }
-    else
-    {
-        SetLastError(ERROR_INVALID_PARAMETER);
-    }
-
-    RtlReleasePebLock();
-
-    return BitSet;
+    /* Done! */
+    return TRUE;
 }
 
 /*
@@ -1045,67 +1073,96 @@
  */
 LPVOID
 WINAPI
-TlsGetValue(DWORD Index)
-{
+TlsGetValue(IN DWORD Index)
+{
+    PTEB Teb;
+
+    /* Get the TEB and clear the last error */
+    Teb = NtCurrentTeb();
+    Teb->LastErrorValue = 0;
+
+    /* Check for simple TLS index */
+    if (Index < TLS_MINIMUM_AVAILABLE)
+    {
+        /* Return it */
+        return Teb->TlsSlots[Index];
+    }
+
+    /* Check for valid index */
     if (Index >= TLS_EXPANSION_SLOTS + TLS_MINIMUM_AVAILABLE)
     {
+        /* Fail */
         SetLastErrorByStatus(STATUS_INVALID_PARAMETER);
         return NULL;
     }
 
-    SetLastError(NO_ERROR);
-
-    if (Index >= TLS_MINIMUM_AVAILABLE)
-    {
-        /* The expansion slots are allocated on demand, so check for it. */
-        if (NtCurrentTeb()->TlsExpansionSlots == NULL)
-            return NULL;
-        return NtCurrentTeb()->TlsExpansionSlots[Index - TLS_MINIMUM_AVAILABLE];
-    }
-    else
-    {
-        return NtCurrentTeb()->TlsSlots[Index];
-    }
-}
-
-/*
- * @implemented
- */
-BOOL
-WINAPI
-TlsSetValue(DWORD Index,
-            LPVOID Value)
-{
-    if (Index >= TLS_EXPANSION_SLOTS + TLS_MINIMUM_AVAILABLE)
-    {
+    /* The expansion slots are allocated on demand, so check for it. */
+    Teb->LastErrorValue = 0;
+    if (!Teb->TlsExpansionSlots) return NULL;
+
+    /* Return the value from the expansion slots */
+    return Teb->TlsExpansionSlots[Index - TLS_MINIMUM_AVAILABLE];
+}
+
+/*
+ * @implemented
+ */
+BOOL
+WINAPI
+TlsSetValue(IN DWORD Index,
+            IN LPVOID Value)
+{
+    DWORD TlsIndex;
+    PTEB Teb = NtCurrentTeb();
+
+    /* Check for simple TLS index */
+    if (Index < TLS_MINIMUM_AVAILABLE)
+    {
+        /* Return it */
+        Teb->TlsSlots[Index] = Value;
+        return TRUE;
+    }
+
+    /* Check if this is an expansion slot */
+    TlsIndex = Index - TLS_MINIMUM_AVAILABLE;
+    if (TlsIndex >= TLS_EXPANSION_SLOTS)
+    {
+        /* Fail */
         SetLastErrorByStatus(STATUS_INVALID_PARAMETER);
         return FALSE;
     }
 
-    if (Index >= TLS_MINIMUM_AVAILABLE)
-    {
-        if (NtCurrentTeb()->TlsExpansionSlots == NULL)
+    /* Do we not have expansion slots? */
+    if (!Teb->TlsExpansionSlots)
+    {
+        /* Get the PEB lock to see if we still need them */
+        RtlAcquirePebLock();
+        if (!Teb->TlsExpansionSlots)
         {
-            NtCurrentTeb()->TlsExpansionSlots = HeapAlloc(GetProcessHeap(),
-                                                          HEAP_ZERO_MEMORY,
-                                                          TLS_EXPANSION_SLOTS *
-                                                          sizeof(PVOID));
-
-            if (NtCurrentTeb()->TlsExpansionSlots == NULL)
+            /* Allocate them */
+            Teb->TlsExpansionSlots = RtlAllocateHeap(RtlGetProcessHeap(),
+                                                     HEAP_ZERO_MEMORY,
+                                                     TLS_EXPANSION_SLOTS *
+                                                     sizeof(PVOID));
+            if (!Teb->TlsExpansionSlots)
             {
-                SetLastError(ERROR_NOT_ENOUGH_MEMORY);
+                /* Fail */
+                RtlReleasePebLock();
+                SetLastErrorByStatus(STATUS_NO_MEMORY);
                 return FALSE;
             }
         }
 
-        NtCurrentTeb()->TlsExpansionSlots[Index - TLS_MINIMUM_AVAILABLE] = Value;
-    }
-    else
-    {
-        NtCurrentTeb()->TlsSlots[Index] = Value;
-    }
-
-    return TRUE;
-}
+        /* Release the lock */
+        RtlReleasePebLock();
+    }
+
+    /* Write the value */
+    Teb->TlsExpansionSlots[TlsIndex] = Value;
+
+    /* Success */
+    return TRUE;
+}
+
 
 /* EOF */