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, <sginsberg(a)svn.reactos.org> 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/…
>
>  ==============================================================================
>  --- 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/…
>
>  ==============================================================================
>  --- 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/…
>
>  ==============================================================================
>  --- 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
>
>   //
>
>
>