[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
ionucu at videotron.ca
Sat Oct 10 01:20:51 UTC 2015
How did our Fiber tests ever pass?
Best regards,
Alex Ionescu
On Thu, Oct 8, 2015 at 9:26 AM, <sginsberg at 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/fiber.c?rev=69466&r1=69465&r2=69466&view=diff
>
> ==============================================================================
> --- 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/i386/fiber.S?rev=69466&r1=69465&r2=69466&view=diff
>
> ==============================================================================
> --- 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 at 0
> -.PROC _BaseFiberStartup at 0
> - /* Frame pointer is zeroed */
> +FUNC _BaseFiberStartup at 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 at 8
> -.ENDP
> + /* Start the thread with our parameters */
> + push dword ptr [eax+FIBER_CONTEXT_EBX]
> + push dword ptr [eax+FIBER_CONTEXT_EAX]
> + jmp _BaseThreadStartup at 8
> +
> +ENDFUNC
>
>
> PUBLIC _SwitchToFiber at 4
> -_SwitchToFiber at 4:
> +FUNC _SwitchToFiber at 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/utils.c?rev=69466&r1=69465&r2=69466&view=diff
>
> ==============================================================================
> --- 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.h?rev=69466&r1=69465&r2=69466&view=diff
>
> ==============================================================================
> --- 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=69466&r1=69465&r2=69466&view=diff
>
> ==============================================================================
> --- 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
>
> //
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20151009/3fc01ec8/attachment-0001.html>
More information about the Ros-dev
mailing list