Re: [ros-dev] [ros-diffs] [sginsberg] 69466: [KERNEL32] Fix bug in CreateFiberEx that made it replace the CONTEXT_FULL bits rather than ORing in CONTEXT_FLOATING_POINT when caller wanted FPU state saved. SwitchToFiber check...

[Alex Ionescu]

How did our Fiber tests ever pass?

Best regards,
Alex Ionescu

On Thu, Oct 8, 2015 at 9:26 AM, [e-mail] wrote:
> Author: sginsberg
> Date: Thu Oct  8 16:26:29 2015
> New Revision: 69466
>
> URL: http://svn.reactos.org/svn/reactos?rev=69466&view=rev
> Log:
> [KERNEL32] Fix bug in CreateFiberEx that made it replace the CONTEXT_FULL
> bits rather than ORing in CONTEXT_FLOATING_POINT when caller wanted FPU
> state saved. SwitchToFiber checks for CONTEXT_FULL OR
> CONTEXT_FLOATING_POINT so the save/restore would fail. Moreover, fix
> BaseInitializeContext that was not checking CONTEXT_FLOATING_POINT
> correctly for some fibers and, as a result, not initializing FPU context
> correctly for callers of ConvertThreadToFiberEx. Finally, because trying to
> access address 0x6 is generally a bad idea, fix SwitchToFiber to use the
> correct shared user data offsets. Misc cleanup all around. Bonus: Sort
> TEB/PEB asm offsets and add GDI Batch Count offset, needed soon.
>
> Modified:
>     trunk/reactos/dll/win32/kernel32/client/fiber.c
>     trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
>     trunk/reactos/dll/win32/kernel32/client/utils.c
>     trunk/reactos/include/asm/ks386.template.h
>     trunk/reactos/include/ndk/i386/asm.h
>
> Modified: trunk/reactos/dll/win32/kernel32/client/fiber.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/f...
>
> ==============================================================================
> --- trunk/reactos/dll/win32/kernel32/client/fiber.c     [iso-8859-1]
> (original)
> +++ trunk/reactos/dll/win32/kernel32/client/fiber.c     [iso-8859-1] Thu
> Oct  8 16:26:29 2015
> @@ -27,10 +27,9 @@
>
> VOID
>  WINAPI
> -BaseRundownFls(IN PVOID FlsData)
> +BaseRundownFls(_In_ PVOID FlsData)
>  {
>      /* No FLS support yet */
>
> }
>
> /* PUBLIC FUNCTIONS
> ***********************************************************/
> @@ -55,16 +54,18 @@
>          return FALSE;
>      }
> /* this thread won't run a fiber anymore */
>
> /* This thread won't run a fiber anymore */
> Teb->HasFiberData = FALSE;
> FiberData = Teb->NtTib.FiberData;
> Teb->NtTib.FiberData = NULL;
>
> /* Free the fiber */
> ASSERT(FiberData != NULL);
> RtlFreeHeap(GetProcessHeap(), 0, FiberData);
> /* success */
> RtlFreeHeap(GetProcessHeap(),
>            0,
>            FiberData);
> /* Success */
> return TRUE;
>
> }
>
> @@ -73,15 +74,15 @@
>   */
>  LPVOID
>  WINAPI
> -ConvertThreadToFiberEx(LPVOID lpParameter,
>                   DWORD dwFlags)
>
> +ConvertThreadToFiberEx(_In_opt_ LPVOID lpParameter,
>                   _In_ DWORD dwFlags)
>
> {
>      PTEB Teb;
>      PFIBER Fiber;
>      DPRINT1("Converting Thread to Fiber\n");
>  /* Check for invalid flags */
> if (dwFlags &~ FIBER_FLAG_FLOAT_SWITCH)
> if (dwFlags & ~FIBER_FLAG_FLOAT_SWITCH)
> {
>     /* Fail */
>     SetLastError(ERROR_INVALID_PARAMETER);
>
> @@ -98,9 +99,12 @@
>      }
>  /* Allocate the fiber */
> Fiber = RtlAllocateHeap(RtlGetProcessHeap(), 0, sizeof(FIBER));
> Fiber = RtlAllocateHeap(RtlGetProcessHeap(),
>                        0,
>                        sizeof(FIBER));
> if (!Fiber)
>    {
>    /* Fail */
>    SetLastError(ERROR_NOT_ENOUGH_MEMORY);
>    return NULL;
> }
>
> @@ -114,13 +118,11 @@
>      Fiber->FlsData = Teb->FlsData;
>      Fiber->GuaranteedStackBytes = Teb->GuaranteedStackBytes;
>      Fiber->ActivationContextStackPointer =
> Teb->ActivationContextStackPointer;
> Fiber->FiberContext.ContextFlags = CONTEXT_FULL;
> /* Save FPU State if requested */
> if (dwFlags & FIBER_FLAG_FLOAT_SWITCH)
> {
>    Fiber->FiberContext.ContextFlags |= CONTEXT_FLOATING_POINT;
> }
> /* Save FPU State if requested, otherwise just the basic registers */
> Fiber->FiberContext.ContextFlags = (dwFlags &
>
> FIBER_FLAG_FLOAT_SWITCH) ?
>                                   (CONTEXT_FULL |
>
> CONTEXT_FLOATING_POINT) :
>                                   CONTEXT_FULL;
>
> /* Associate the fiber to the current thread */
>    Teb->NtTib.FiberData = Fiber;
>
> @@ -135,10 +137,11 @@
>   */
>  LPVOID
>  WINAPI
> -ConvertThreadToFiber(LPVOID lpParameter)
> +ConvertThreadToFiber(_In_opt_ LPVOID lpParameter)
>  {
>      /* Call the newer function */
> return ConvertThreadToFiberEx(lpParameter, 0);
> return ConvertThreadToFiberEx(lpParameter,
>                              0);
>
> }
>
> /*
> @@ -146,12 +149,16 @@
>   */
>  LPVOID
>  WINAPI
> -CreateFiber(SIZE_T dwStackSize,
>        LPFIBER_START_ROUTINE lpStartAddress,
>        LPVOID lpParameter)
>
> +CreateFiber(_In_ SIZE_T dwStackSize,
>        _In_ LPFIBER_START_ROUTINE lpStartAddress,
>        _In_opt_ LPVOID lpParameter)
>
> {
>      /* Call the Newer Function */
> return CreateFiberEx(dwStackSize, 0, 0, lpStartAddress, lpParameter);
> return CreateFiberEx(dwStackSize,
>                     0,
>                     0,
>                     lpStartAddress,
>                     lpParameter);
>
> }
>
> /*
> @@ -159,20 +166,20 @@
>   */
>  LPVOID
>  WINAPI
> -CreateFiberEx(SIZE_T dwStackCommitSize,
>          SIZE_T dwStackReserveSize,
>          DWORD dwFlags,
>          LPFIBER_START_ROUTINE lpStartAddress,
>          LPVOID lpParameter)
>
> +CreateFiberEx(_In_ SIZE_T dwStackCommitSize,
>          _In_ SIZE_T dwStackReserveSize,
>          _In_ DWORD dwFlags,
>          _In_ LPFIBER_START_ROUTINE lpStartAddress,
>          _In_opt_ LPVOID lpParameter)
>
> {
>      PFIBER Fiber;
>      NTSTATUS Status;
>      INITIAL_TEB InitialTeb;
> PACTIVATION_CONTEXT_STACK ActivationContextStackPointer = NULL;
>
> PACTIVATION_CONTEXT_STACK ActivationContextStackPointer;
> DPRINT("Creating Fiber\n");
>
> /* Check for invalid flags */
> if (dwFlags &~ FIBER_FLAG_FLOAT_SWITCH)
> if (dwFlags & ~FIBER_FLAG_FLOAT_SWITCH)
> {
>     /* Fail */
>     SetLastError(ERROR_INVALID_PARAMETER);
>
> @@ -180,6 +187,7 @@
>      }
>  /* Allocate the Activation Context Stack */
> ActivationContextStackPointer = NULL;
> Status =
>
> RtlAllocateActivationContextStack(&ActivationContextStackPointer);
>      if (!NT_SUCCESS(Status))
>      {
> @@ -189,7 +197,9 @@
>      }
>  /* Allocate the fiber */
> Fiber = RtlAllocateHeap(RtlGetProcessHeap(), 0, sizeof(FIBER));
> Fiber = RtlAllocateHeap(RtlGetProcessHeap(),
>                        0,
>                        sizeof(FIBER));
> if (!Fiber)
>    {
>        /* Free the activation context stack */
>
> @@ -208,7 +218,9 @@
>      if (!NT_SUCCESS(Status))
>      {
>          /* Free the fiber */
>    RtlFreeHeap(GetProcessHeap(), 0, Fiber);
>    RtlFreeHeap(GetProcessHeap(),
>                0,
>                Fiber);
>
>    /* Free the activation context stack */
>    RtlFreeActivationContextStack(ActivationContextStackPointer);
>
> @@ -219,7 +231,8 @@
>      }
>  /* Clear the context */
> RtlZeroMemory(&Fiber->FiberContext, sizeof(CONTEXT));
>
> RtlZeroMemory(&Fiber->FiberContext,
>              sizeof(CONTEXT));
>
> /* Copy the data into the fiber */
>    Fiber->StackBase = InitialTeb.StackBase;
>
> @@ -230,12 +243,13 @@
>      Fiber->GuaranteedStackBytes = 0;
>      Fiber->FlsData = NULL;
>      Fiber->ActivationContextStackPointer = ActivationContextStackPointer;
> Fiber->FiberContext.ContextFlags = CONTEXT_FULL;
> /* Save FPU State if requested */
> Fiber->FiberContext.ContextFlags = (dwFlags &
>
> FIBER_FLAG_FLOAT_SWITCH) ? CONTEXT_FLOATING_POINT : 0;
> /* initialize the context for the fiber */
> /* Save FPU State if requested, otherwise just the basic registers */
> Fiber->FiberContext.ContextFlags = (dwFlags &
>
> FIBER_FLAG_FLOAT_SWITCH) ?
>                                   (CONTEXT_FULL |
>
> CONTEXT_FLOATING_POINT) :
>                                   CONTEXT_FULL;
> /* Initialize the context for the fiber */
> BaseInitializeContext(&Fiber->FiberContext,
>                       lpParameter,
>                       lpStartAddress,
>
> @@ -251,17 +265,24 @@
>   */
>  VOID
>  WINAPI
> -DeleteFiber(LPVOID lpFiber)
> -{
> SIZE_T Size = 0;
> PFIBER Fiber = (PFIBER)lpFiber;
>
> +DeleteFiber(_In_ LPVOID lpFiber)
> +{
> SIZE_T Size;
> PFIBER Fiber;
> PTEB Teb;
> /* First, exit the thread */
> /* Are we deleting ourselves? */
> Teb = NtCurrentTeb();
> if ((Teb->HasFiberData) && (Teb->NtTib.FiberData == Fiber))
>
> ExitThread(1);
> /* Now de-allocate the stack */
> Fiber = (PFIBER)lpFiber;
> if ((Teb->HasFiberData) &&
>    (Teb->NtTib.FiberData == Fiber))
> {
>    /* Just exit */
>    ExitThread(1);
> }
> /* Not ourselves, de-allocate the stack */
> Size = 0 ;
> NtFreeVirtualMemory(NtCurrentProcess(),
>                     &Fiber->DeallocationStack,
>                     &Size,
>
> @@ -274,7 +295,9 @@
>      RtlFreeActivationContextStack(Fiber->ActivationContextStackPointer);
>  /* Free the fiber data */
> RtlFreeHeap(GetProcessHeap(), 0, lpFiber);
> RtlFreeHeap(GetProcessHeap(),
>            0,
>            lpFiber);
>
> }
>
> /*
> @@ -284,6 +307,7 @@
>  WINAPI
>  IsThreadAFiber(VOID)
>  {
> /* Return flag in the TEB */
> return NtCurrentTeb()->HasFiberData;
>
> }
>
> @@ -349,7 +373,8 @@
>   */
>  BOOL
>  WINAPI
> -FlsSetValue(DWORD dwFlsIndex, PVOID lpFlsData)
> +FlsSetValue(DWORD dwFlsIndex,
>        PVOID lpFlsData)
>
> {
>      PVOID *ppFlsSlots;
>      TEB *pTeb = NtCurrentTeb();
>
> Modified: trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/i...
>
> ==============================================================================
> --- trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
> [iso-8859-1] (original)
> +++ trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
> [iso-8859-1] Thu Oct  8 16:26:29 2015
> @@ -15,21 +15,26 @@
>  .code
>
> PUBLIC _BaseFiberStartup@0
> -.PROC _BaseFiberStartup@0
> /* Frame pointer is zeroed */
>
> +FUNC _BaseFiberStartup@0
>      FPO 0, 0, 0, 0, 0, FRAME_FPO
>
> /* Note that EBP is already zeroed for us during fiber creation */
>
> /* Get the fiber data */
> mov eax, fs:[TEB_FIBER_DATA]
> push dword ptr [eax+FIBER_CONTEXT_EBX] /* Parameter */
> push dword ptr [eax+FIBER_CONTEXT_EAX] /* Start Address */
> call _BaseThreadStartup@8
>
> -.ENDP
> /* Start the thread with our parameters */
> push dword ptr [eax+FIBER_CONTEXT_EBX]
> push dword ptr [eax+FIBER_CONTEXT_EAX]
> jmp _BaseThreadStartup@8
>
> +ENDFUNC
>
> PUBLIC _SwitchToFiber@4
> -_SwitchToFiber@4:
> +FUNC _SwitchToFiber@4
> FPO 0, 0, 0, 0, 0, FRAME_FPO
> /* Get the TEB */
>    mov edx, fs:[TEB_SELF]
>
> @@ -51,7 +56,7 @@
>      fnstcw [eax+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]
>  /* Check if the CPU supports SIMD MXCSR State Save */
> cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
> cmp byte ptr ds:[USER_SHARED_DATA +
>
> USER_SHARED_DATA_PROCESSOR_FEATURES + PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
>      jnz NoFpuStateSave
>      stmxcsr [eax+FIBER_CONTEXT_DR6]
>
> @@ -109,13 +114,13 @@
>  StatusWordChanged:
>  /* Load the new one */
> mov word ptr [ecx+FIBER_CONTEXT_FLOAT_SAVE_TAG_WORD], HEX(0FFFF)
> mov word ptr [ecx+FIBER_CONTEXT_FLOAT_SAVE_TAG_WORD], HEX(FFFF)
> fldenv [ecx+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]
>
> ControlWordEqual:
>  /* Load the new one */
> cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
> cmp byte ptr ds:[USER_SHARED_DATA +
>
> USER_SHARED_DATA_PROCESSOR_FEATURES + PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
>      jnz NoFpuStateRestore
>      ldmxcsr [ecx+FIBER_CONTEXT_DR6]
>
> @@ -136,6 +141,7 @@
>      mov esp, [ecx+FIBER_CONTEXT_ESP]
>      ret 4
>
> +ENDFUNC
>
> END
>  /* EOF */
>
> Modified: trunk/reactos/dll/win32/kernel32/client/utils.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/u...
>
> ==============================================================================
> --- trunk/reactos/dll/win32/kernel32/client/utils.c     [iso-8859-1]
> (original)
> +++ trunk/reactos/dll/win32/kernel32/client/utils.c     [iso-8859-1] Thu
> Oct  8 16:26:29 2015
> @@ -548,7 +548,7 @@
>      /* Is FPU state required? */
>      Context->ContextFlags |= ContextFlags;
>    if (ContextFlags == CONTEXT_FLOATING_POINT)
>    if ((ContextFlags & CONTEXT_FLOATING_POINT) ==
>
> CONTEXT_FLOATING_POINT)
>          {
>              /* Set an initial state */
>              Context->FloatSave.ControlWord = 0x27F;
>
> Modified: trunk/reactos/include/asm/ks386.template.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/include/asm/ks386.template....
>
> ==============================================================================
> --- trunk/reactos/include/asm/ks386.template.h  [iso-8859-1] (original)
> +++ trunk/reactos/include/asm/ks386.template.h  [iso-8859-1] Thu Oct  8
> 16:26:29 2015
> @@ -724,17 +724,20 @@
>
> HEADER("TEB"),
>  OFFSET(TEB_EXCEPTION_LIST, TEB, NtTib.ExceptionList),
> +OFFSET(TEB_STACK_BASE, TEB, NtTib.StackBase),
>  OFFSET(TEB_STACK_LIMIT, TEB, NtTib.StackLimit),
> -OFFSET(TEB_STACK_BASE, TEB, NtTib.StackBase),
> +OFFSET(TEB_FIBER_DATA, TEB, NtTib.FiberData),
>  OFFSET(TEB_SELF, TEB, NtTib.Self),
> -OFFSET(TEB_FIBER_DATA, TEB, NtTib.FiberData),
>  OFFSET(TEB_PEB, TEB, ProcessEnvironmentBlock),
>  OFFSET(TEB_EXCEPTION_CODE, TEB, ExceptionCode),
> +OFFSET(TEB_ACTIVATION_CONTEXT_STACK_POINTER, TEB,
> ActivationContextStackPointer),
> +OFFSET(TEB_DEALLOCATION_STACK, TEB, DeallocationStack),
> +OFFSET(TEB_GDI_BATCH_COUNT, TEB, GdiBatchCount),
> +OFFSET(TEB_GUARANTEED_STACK_BYTES, TEB, GuaranteedStackBytes),
> +OFFSET(TEB_FLS_DATA, TEB, FlsData),
>
> +HEADER("PEB"),
>  OFFSET(PEB_KERNEL_CALLBACK_TABLE, PEB, KernelCallbackTable),
> -OFFSET(TEB_FLS_DATA, TEB, FlsData),
> -OFFSET(TEB_ACTIVATION_CONTEXT_STACK_POINTER, TEB,
> ActivationContextStackPointer),
> -OFFSET(TEB_GUARANTEED_STACK_BYTES, TEB, GuaranteedStackBytes),
> -OFFSET(TEB_DEALLOCATION_STACK, TEB, DeallocationStack),
>
> HEADER("Misc"),
>  CONSTANT(NPX_FRAME_LENGTH),
> @@ -754,4 +757,6 @@
>  CONSTANT(CONTEXT_ALIGNED_SIZE),
>  CONSTANT(PROCESSOR_FEATURE_FXSR),
>  CONSTANT(KUSER_SHARED_SYSCALL_RET),
>
> +CONSTANT(USER_SHARED_DATA),
> +CONSTANT(USER_SHARED_DATA_PROCESSOR_FEATURES),
>
> Modified: trunk/reactos/include/ndk/i386/asm.h
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/include/ndk/i386/asm.h?rev=...
>
> ==============================================================================
> --- trunk/reactos/include/ndk/i386/asm.h        [iso-8859-1] (original)
> +++ trunk/reactos/include/ndk/i386/asm.h        [iso-8859-1] Thu Oct  8
> 16:26:29 2015
> @@ -315,13 +315,14 @@
>  #define FRAME_EDITED                            0xFFF8
>
> //
> -// KUSER_SHARED_DATA Offsets
> +// USER_SHARED_DATA Offsets
>  //
>  #ifdef __ASM__
>  #define USER_SHARED_DATA                        0xFFDF0000
>  #endif
>  #define USER_SHARED_DATA_INTERRUPT_TIME         0x8
>  #define USER_SHARED_DATA_SYSTEM_TIME            0x14
> +#define USER_SHARED_DATA_PROCESSOR_FEATURES     0x274
>  #define USER_SHARED_DATA_TICK_COUNT             0x320
>
> //