ShuriZma
/
Cxbx-Reloaded
mirror of https://github.com/Cxbx-Reloaded/Cxbx-Reloaded.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fixed an issue in WaitApc where the wait block was not removed when using a zero timeout or when satisfied by a user APC + properly lock the wait block operations to avoid a race between SatisfyWait and KiTimerExpiration

This commit is contained in:
ergo720 2023-03-31 15:49:40 +02:00 committed by RadWolfie
parent 86542c9f2e
commit 889040c56a
8 changed files with 111 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
src/core/hle/XAPI/Xapi.cpp
View File

@ -962,17 +962,11 @@ xbox::dword_xt WINAPI xbox::EMUPATCH(SignalObjectAndWait)
PKTHREAD kThread = KeGetCurrentThread();
kThread->WaitStatus = X_STATUS_SUCCESS;
if (!Timeout || (Timeout->QuadPart == 0)) {
// Use the built-in ktimer as a dummy wait object, so that KiUnwaitThreadAndLock can still work
xbox::PKWAIT_BLOCK WaitBlock = &kThread->TimerWaitBlock;
kThread->WaitBlockList = WaitBlock;
xbox::PKTIMER Timer = &kThread->Timer;
WaitBlock->NextWaitBlock = WaitBlock;
Timer->Header.WaitListHead.Flink = &WaitBlock->WaitListEntry;
Timer->Header.WaitListHead.Blink = &WaitBlock->WaitListEntry;
if (!AddWaitObject(kThread, Timeout)) {
RETURN(WAIT_TIMEOUT);
}
xbox::ntstatus_xt status = WaitApc<true>([hObjectToSignal, hObjectToWaitOn, bAlertable, kThread]() -> std::optional<DWORD> {
xbox::ntstatus_xt status = WaitApc<true>([hObjectToSignal, hObjectToWaitOn, bAlertable](xbox::PKTHREAD kThread) -> std::optional<DWORD> {
DWORD dwRet = SignalObjectAndWait(hObjectToSignal, hObjectToWaitOn, 0, bAlertable);
if (dwRet == WAIT_TIMEOUT) {
return std::nullopt;
@ -988,8 +982,8 @@ xbox::dword_xt WINAPI xbox::EMUPATCH(SignalObjectAndWait)
default: Status = X_STATUS_INVALID_HANDLE;
}
xbox::KiUnwaitThreadAndLock(kThread, Status, 0);
return std::make_optional<ntstatus_xt>(Status);
}, Timeout, bAlertable, UserMode);
return std::make_optional<ntstatus_xt>(kThread->WaitStatus);
}, Timeout, bAlertable, UserMode, kThread);
xbox::dword_xt ret;
switch (status)

27
src/core/kernel/exports/EmuKrnl.cpp
View File

@ -180,6 +180,33 @@ void CallSoftwareInterrupt(const xbox::KIRQL SoftwareIrql)
HalInterruptRequestRegister ^= (1 << SoftwareIrql);
}
bool AddWaitObject(xbox::PKTHREAD kThread, xbox::PLARGE_INTEGER Timeout)
{
// Use the built-in ktimer as a dummy wait object, so that KiUnwaitThreadAndLock can still work
xbox::KiTimerLock();
xbox::PKWAIT_BLOCK WaitBlock = &kThread->TimerWaitBlock;
kThread->WaitBlockList = WaitBlock;
xbox::PKTIMER Timer = &kThread->Timer;
WaitBlock->NextWaitBlock = WaitBlock;
Timer->Header.WaitListHead.Flink = &WaitBlock->WaitListEntry;
Timer->Header.WaitListHead.Blink = &WaitBlock->WaitListEntry;
if (Timeout && Timeout->QuadPart) {
// Setup a timer so that KiTimerExpiration can discover the timeout and yield to us.
// Otherwise, we will only be able to discover the timeout when Windows decides to schedule us again, and testing shows that
// tends to happen much later than the due time
if (xbox::KiInsertTreeTimer(Timer, *Timeout) == FALSE) {
// Sanity check: set WaitBlockList to nullptr so that we can catch the case where a waiter starts a new wait but forgets to setup a new wait block. This
// way, we will crash instead of silently using the pointer to the old block
kThread->WaitBlockList = xbox::zeroptr;
xbox::KiTimerUnlock();
return false;
}
}
kThread->State = xbox::Waiting;
xbox::KiTimerUnlock();
return true;
}
// This masks have been verified to be correct against a kernel dump
const DWORD IrqlMasks[] = {
0xFFFFFFFE, // IRQL 0

41
src/core/kernel/exports/EmuKrnl.h
View File

@ -108,11 +108,12 @@ extern HalSystemInterrupt HalSystemInterrupts[MAX_BUS_INTERRUPT_LEVEL + 1];
bool DisableInterrupts();
void RestoreInterruptMode(bool value);
void CallSoftwareInterrupt(const xbox::KIRQL SoftwareIrql);
bool AddWaitObject(xbox::PKTHREAD kThread, xbox::PLARGE_INTEGER Timeout);
template<typename T>
std::optional<xbox::ntstatus_xt> SatisfyWait(T &&Lambda, xbox::PKTHREAD kThread, xbox::boolean_xt Alertable, xbox::char_xt WaitMode)
{
if (const auto ret = Lambda()) {
if (const auto ret = Lambda(kThread)) {
return ret;
}
@ -129,30 +130,22 @@ std::optional<xbox::ntstatus_xt> SatisfyWait(T &&Lambda, xbox::PKTHREAD kThread,
(Alertable == TRUE) &&
(WaitMode == xbox::UserMode)) {
xbox::KiExecuteUserApc();
return X_STATUS_USER_APC;
xbox::KiUnwaitThreadAndLock(kThread, X_STATUS_USER_APC, 0);
return kThread->WaitStatus;
}
return std::nullopt;
}
template<bool host_wait, typename T>
xbox::ntstatus_xt WaitApc(T &&Lambda, xbox::PLARGE_INTEGER Timeout, xbox::boolean_xt Alertable, xbox::char_xt WaitMode)
xbox::ntstatus_xt WaitApc(T &&Lambda, xbox::PLARGE_INTEGER Timeout, xbox::boolean_xt Alertable, xbox::char_xt WaitMode, xbox::PKTHREAD kThread)
{
// NOTE1: kThread->Alerted is currently never set. When the alerted mechanism is implemented, the alerts should
// also interrupt the wait.
// NOTE2: kThread->State should only be set when this function performs a host wait. Otherwise, a race condition exists where the guest satisfies the wait
// and calls KiUnwaitThread (which sets the state to Ready), before we set the Waiting state here. This will then cause a deadlock, since the wait here expects
// a Ready state instead, which was previosuly overwritten by Waiting. Test case: PsCreateSystemThreadEx when it suspends/resumes the child thread with
// KeSuspend/ResumeThreadEx.
xbox::ntstatus_xt status;
xbox::PKTHREAD kThread = xbox::KeGetCurrentThread();
if (Timeout == nullptr) {
// No timout specified, so this is an infinite wait until an alert, a user apc or the object(s) become(s) signalled
if constexpr (host_wait) {
kThread->State = xbox::Waiting;
}
while (true) {
if (const auto ret = SatisfyWait(Lambda, kThread, Alertable, WaitMode)) {
status = *ret;
@ -163,34 +156,20 @@ xbox::ntstatus_xt WaitApc(T &&Lambda, xbox::PLARGE_INTEGER Timeout, xbox::boolea
}
}
else if (Timeout->QuadPart == 0) {
assert(host_wait);
// A zero timeout means that we only have to check the conditions once and then return immediately if they are not satisfied
if (const auto ret = SatisfyWait(Lambda, kThread, Alertable, WaitMode)) {
status = *ret;
}
else {
status = X_STATUS_TIMEOUT;
// If the wait failed, then always remove the wait block. Note that this can only happen with host waits, since guest waits never call us at all
// when Timeout->QuadPart == 0. Test case: Halo 2 (sporadically when playing the intro video)
xbox::KiUnwaitThreadAndLock(kThread, X_STATUS_TIMEOUT, 0);
status = kThread->WaitStatus;
}
}
else {
// A non-zero timeout means we have to check the conditions until we reach the requested time
if constexpr (host_wait) {
// Setup a timer if it wasn't already by our caller. This is necessary because in this way, KiTimerExpiration can discover the timeout
// and yield to us. Otherwise, we will only be able to discover the timeout when Windows decides to schedule us again, and testing shows that
// tends to happen much later than the due time
xbox::KiTimerLock();
xbox::PKWAIT_BLOCK WaitBlock = &kThread->TimerWaitBlock;
kThread->WaitBlockList = WaitBlock;
xbox::PKTIMER Timer = &kThread->Timer;
WaitBlock->NextWaitBlock = WaitBlock;
Timer->Header.WaitListHead.Flink = &WaitBlock->WaitListEntry;
Timer->Header.WaitListHead.Blink = &WaitBlock->WaitListEntry;
if (xbox::KiInsertTreeTimer(Timer, *Timeout) == FALSE) {
xbox::KiTimerUnlock();
return X_STATUS_TIMEOUT;
}
kThread->State = xbox::Waiting;
xbox::KiTimerUnlock();
}
while (true) {
if (const auto ret = SatisfyWait(Lambda, kThread, Alertable, WaitMode)) {
status = *ret;

50
src/core/kernel/exports/EmuKrnlKe.cpp
View File

@ -140,6 +140,7 @@ xbox::void_xt xbox::KeResumeThreadEx
// This is only to be used to synchronize new thread creation with the thread that spawned it
Thread->SuspendSemaphore.Header.SignalState = 1;
KiWaitListLock();
KiWaitTest(&Thread->SuspendSemaphore, 0);
}
@ -735,17 +736,11 @@ XBSYSAPI EXPORTNUM(99) xbox::ntstatus_xt NTAPI xbox::KeDelayExecutionThread
PKTHREAD kThread = KeGetCurrentThread();
kThread->WaitStatus = X_STATUS_SUCCESS;
if (!Interval || (Interval->QuadPart == 0)) {
// Use the built-in ktimer as a dummy wait object, so that KiUnwaitThreadAndLock can still work
xbox::PKWAIT_BLOCK WaitBlock = &kThread->TimerWaitBlock;
kThread->WaitBlockList = WaitBlock;
xbox::PKTIMER Timer = &kThread->Timer;
WaitBlock->NextWaitBlock = WaitBlock;
Timer->Header.WaitListHead.Flink = &WaitBlock->WaitListEntry;
Timer->Header.WaitListHead.Blink = &WaitBlock->WaitListEntry;
if (!AddWaitObject(kThread, Interval)) {
RETURN(X_STATUS_TIMEOUT);
}
xbox::ntstatus_xt ret = WaitApc<true>([Alertable, kThread]() -> std::optional<ntstatus_xt> {
xbox::ntstatus_xt ret = WaitApc<true>([Alertable](xbox::PKTHREAD kThread) -> std::optional<ntstatus_xt> {
NtDll::LARGE_INTEGER ExpireTime;
ExpireTime.QuadPart = 0;
NTSTATUS Status = NtDll::NtDelayExecution(Alertable, &ExpireTime);
@ -753,12 +748,11 @@ XBSYSAPI EXPORTNUM(99) xbox::ntstatus_xt NTAPI xbox::KeDelayExecutionThread
if (Status >= 0 && Status != STATUS_ALERTED && Status != STATUS_USER_APC) {
return std::nullopt;
}
EmuLog(LOG_LEVEL::DEBUG, "KeDelayExecutionThread -> Staus: %X", Status);
// If the wait was satisfied with the host, then also unwait the thread on the guest side, to be sure to remove WaitBlocks that might have been added
// to the thread. Test case: Steel Battalion
xbox::KiUnwaitThreadAndLock(kThread, Status, 0);
return std::make_optional<ntstatus_xt>(Status);
}, Interval, Alertable, WaitMode);
return std::make_optional<ntstatus_xt>(kThread->WaitStatus);
}, Interval, Alertable, WaitMode, kThread);
if (ret == X_STATUS_TIMEOUT) {
// NOTE: this function considers a timeout a success
@ -1361,9 +1355,14 @@ XBSYSAPI EXPORTNUM(123) xbox::long_xt NTAPI xbox::KePulseEvent
}
LONG OldState = Event->Header.SignalState;
KiWaitListLock();
if ((OldState == 0) && (IsListEmpty(&Event->Header.WaitListHead) == FALSE)) {
Event->Header.SignalState = 1;
KiWaitTest(Event, Increment);
std::this_thread::yield();
}
else {
KiWaitListUnlock();
}
Event->Header.SignalState = 0;
@ -1548,8 +1547,13 @@ XBSYSAPI EXPORTNUM(132) xbox::long_xt NTAPI xbox::KeReleaseSemaphore
}
Semaphore->Header.SignalState = adjusted_signalstate;
KiWaitListLock();
if ((initial_state == 0) && (IsListEmpty(&Semaphore->Header.WaitListHead) == FALSE)) {
KiWaitTest(&Semaphore->Header, Increment);
std::this_thread::yield();
}
else {
KiWaitListUnlock();
}
if (Wait) {
@ -1773,7 +1777,9 @@ XBSYSAPI EXPORTNUM(140) xbox::ulong_xt NTAPI xbox::KeResumeThread
if (Thread->SuspendCount == 0) {
#if 0
++Thread->SuspendSemaphore.Header.SignalState;
KiWaitListLock();
KiWaitTest(&Thread->SuspendSemaphore, 0);
std::this_thread::yield();
#else
if (const auto &nativeHandle = GetNativeHandle<true>(reinterpret_cast<PETHREAD>(Thread)->UniqueThread)) {
ResumeThread(*nativeHandle);
@ -1923,7 +1929,9 @@ XBSYSAPI EXPORTNUM(145) xbox::long_xt NTAPI xbox::KeSetEvent
}
LONG OldState = Event->Header.SignalState;
KiWaitListLock();
if (IsListEmpty(&Event->Header.WaitListHead) != FALSE) {
KiWaitListUnlock();
Event->Header.SignalState = 1;
} else {
PKWAIT_BLOCK WaitBlock = CONTAINING_RECORD(Event->Header.WaitListHead.Flink, KWAIT_BLOCK, WaitListEntry);
@ -1933,8 +1941,12 @@ XBSYSAPI EXPORTNUM(145) xbox::long_xt NTAPI xbox::KeSetEvent
Event->Header.SignalState = 1;
KiWaitTest(Event, Increment);
}
else {
KiWaitListUnlock();
}
} else {
KiUnwaitThreadAndLock(WaitBlock->Thread, (NTSTATUS)WaitBlock->WaitKey, Increment);
KiUnwaitThread(WaitBlock->Thread, (NTSTATUS)WaitBlock->WaitKey, Increment);
KiWaitListUnlock();
}
}
@ -1971,6 +1983,7 @@ XBSYSAPI EXPORTNUM(146) xbox::void_xt NTAPI xbox::KeSetEventBoostPriority
return;
}
KiWaitListLock();
if (IsListEmpty(&Event->Header.WaitListHead) != FALSE) {
Event->Header.SignalState = 1;
} else {
@ -1981,8 +1994,9 @@ XBSYSAPI EXPORTNUM(146) xbox::void_xt NTAPI xbox::KeSetEventBoostPriority
}
WaitThread->Quantum = WaitThread->ApcState.Process->ThreadQuantum;
KiUnwaitThreadAndLock(WaitThread, X_STATUS_SUCCESS, 1);
KiUnwaitThread(WaitThread, X_STATUS_SUCCESS, 1);
}
KiWaitListUnlock();
KiUnlockDispatcherDatabase(OldIrql);
}
@ -2396,13 +2410,13 @@ XBSYSAPI EXPORTNUM(158) xbox::ntstatus_xt NTAPI xbox::KeWaitForMultipleObjects
//}
// TODO: Remove this after we have our own scheduler and the above is implemented
WaitStatus = WaitApc<false>([Thread]() -> std::optional<ntstatus_xt> {
WaitStatus = WaitApc<false>([](PKTHREAD Thread) -> std::optional<ntstatus_xt> {
if (Thread->State == Ready) {
// We have been readied to resume execution, so exit the wait
return std::make_optional<ntstatus_xt>(Thread->WaitStatus);
}
return std::nullopt;
}, Timeout, Alertable, WaitMode);
}, Timeout, Alertable, WaitMode, Thread);
break;
}
@ -2584,13 +2598,13 @@ XBSYSAPI EXPORTNUM(159) xbox::ntstatus_xt NTAPI xbox::KeWaitForSingleObject
KiWaitListUnlock();
// TODO: Remove this after we have our own scheduler and the above is implemented
WaitStatus = WaitApc<false>([Thread]() -> std::optional<ntstatus_xt> {
WaitStatus = WaitApc<false>([](PKTHREAD Thread) -> std::optional<ntstatus_xt> {
if (Thread->State == Ready) {
// We have been readied to resume execution, so exit the wait
return std::make_optional<ntstatus_xt>(Thread->WaitStatus);
}
return std::nullopt;
}, Timeout, Alertable, WaitMode);
}, Timeout, Alertable, WaitMode, Thread);
break;
}

44
src/core/kernel/exports/EmuKrnlKi.cpp
View File

@ -524,9 +524,13 @@ xbox::boolean_xt FASTCALL xbox::KiSignalTimer
Timer->Header.SignalState = TRUE;
/* Check if the timer has waiters */
KiWaitListLock();
if (!IsListEmpty(&Timer->Header.WaitListHead))
{
KiWaitTestNoYield(Timer, 0);
KiWaitTest(Timer, 0);
}
else {
KiWaitListUnlock();
}
/* Check if we have a period */
@ -630,9 +634,13 @@ xbox::void_xt NTAPI xbox::KiTimerExpiration
Period = Timer->Period;
/* Check if there are any waiters */
KiWaitListLock();
if (!IsListEmpty(&Timer->Header.WaitListHead))
{
KiWaitTestNoYield(Timer, 0);
KiWaitTest(Timer, 0);
}
else {
KiWaitListUnlock();
}
/* Check if we have a period */
@ -802,9 +810,13 @@ xbox::void_xt FASTCALL xbox::KiTimerListExpire
Period = Timer->Period;
/* Check if there's any waiters */
KiWaitListLock();
if (!IsListEmpty(&Timer->Header.WaitListHead))
{
KiWaitTestNoYield(Timer, 0);
KiWaitTest(Timer, 0);
}
else {
KiWaitListUnlock();
}
/* Check if we have a period */
@ -1110,7 +1122,7 @@ xbox::boolean_xt xbox::KiInsertQueueApc
return TRUE;
}
xbox::void_xt xbox::KiWaitTestNoYield
xbox::void_xt xbox::KiWaitTest
(
IN PVOID Object,
IN KPRIORITY Increment
@ -1121,8 +1133,9 @@ xbox::void_xt xbox::KiWaitTestNoYield
PKTHREAD WaitThread;
PKMUTANT FirstObject = (PKMUTANT)Object;
ASSERT_WAIT_LIST_LOCKED;
/* Loop the Wait Entries */
KiWaitListLock();
WaitList = &FirstObject->Header.WaitListHead;
WaitEntry = WaitList->Flink;
while ((FirstObject->Header.SignalState > 0) && (WaitEntry != WaitList)) {
@ -1162,18 +1175,6 @@ NextWaitEntry:
KiWaitListUnlock();
}
xbox::void_xt xbox::KiWaitTest
(
IN PVOID Object,
IN KPRIORITY Increment
)
{
KiWaitTestNoYield(Object, Increment);
// Now that we have unwaited all the threads we could, yield
std::this_thread::yield();
}
xbox::void_xt xbox::KiWaitSatisfyAll
(
IN PKWAIT_BLOCK FirstBlock
@ -1216,6 +1217,11 @@ xbox::void_xt xbox::KiUnwaitThread
{
ASSERT_WAIT_LIST_LOCKED;
if (Thread->State != Waiting) {
// Don't do anything if it was already unwaited
return;
}
/* Unlink the thread */
KiUnlinkThread(Thread, WaitStatus);
@ -1280,4 +1286,8 @@ xbox::void_xt xbox::KiUnlinkThread
/* Increment the Queue's active threads */
if (Thread->Queue) Thread->Queue->CurrentCount++;
#endif
// Sanity check: set WaitBlockList to nullptr so that we can catch the case where a waiter starts a new wait but forgets to setup a new wait block. This
// way, we will crash instead of silently using the pointer to the old block
Thread->WaitBlockList = zeroptr;
}

6
src/core/kernel/exports/EmuKrnlKi.h
View File

@ -204,12 +204,6 @@ namespace xbox
IN KPRIORITY Increment
);
void_xt KiWaitTestNoYield
(
IN PVOID Object,
IN KPRIORITY Increment
);
void_xt KiWaitTest
(
IN PVOID Object,

16
src/core/kernel/exports/EmuKrnlNt.cpp
View File

@ -2233,17 +2233,11 @@ XBSYSAPI EXPORTNUM(235) xbox::ntstatus_xt NTAPI xbox::NtWaitForMultipleObjectsEx
PKTHREAD kThread = KeGetCurrentThread();
kThread->WaitStatus = X_STATUS_SUCCESS;
if (!Timeout || (Timeout->QuadPart == 0)) {
// Use the built-in ktimer as a dummy wait object, so that KiUnwaitThreadAndLock can still work
xbox::PKWAIT_BLOCK WaitBlock = &kThread->TimerWaitBlock;
kThread->WaitBlockList = WaitBlock;
xbox::PKTIMER Timer = &kThread->Timer;
WaitBlock->NextWaitBlock = WaitBlock;
Timer->Header.WaitListHead.Flink = &WaitBlock->WaitListEntry;
Timer->Header.WaitListHead.Blink = &WaitBlock->WaitListEntry;
if (!AddWaitObject(kThread, Timeout)) {
RETURN(X_STATUS_TIMEOUT);
}
xbox::ntstatus_xt ret = WaitApc<true>([Count, &nativeHandles, WaitType, Alertable, kThread]() -> std::optional<ntstatus_xt> {
xbox::ntstatus_xt ret = WaitApc<true>([Count, &nativeHandles, WaitType, Alertable](xbox::PKTHREAD kThread) -> std::optional<ntstatus_xt> {
NtDll::LARGE_INTEGER ExpireTime;
ExpireTime.QuadPart = 0;
NTSTATUS Status = NtDll::NtWaitForMultipleObjects(
@ -2258,8 +2252,8 @@ XBSYSAPI EXPORTNUM(235) xbox::ntstatus_xt NTAPI xbox::NtWaitForMultipleObjectsEx
// If the wait was satisfied with the host, then also unwait the thread on the guest side, to be sure to remove WaitBlocks that might have been added
// to the thread. Test case: Steel Battalion
xbox::KiUnwaitThreadAndLock(kThread, Status, 0);
return std::make_optional<ntstatus_xt>(Status);
}, Timeout, Alertable, WaitMode);
return std::make_optional<ntstatus_xt>(kThread->WaitStatus);
}, Timeout, Alertable, WaitMode, kThread);
RETURN(ret);
}

5
src/core/kernel/exports/EmuKrnlPs.cpp
View File

@ -506,8 +506,13 @@ XBSYSAPI EXPORTNUM(258) xbox::void_xt NTAPI xbox::PsTerminateSystemThread
KeQuerySystemTime(&eThread->ExitTime);
eThread->ExitStatus = ExitStatus;
eThread->Tcb.Header.SignalState = 1;
KiWaitListLock();
if (!IsListEmpty(&eThread->Tcb.Header.WaitListHead)) {
KiWaitTest((PVOID)&eThread->Tcb, 0);
std::this_thread::yield();
}
else {
KiWaitListUnlock();
}
if (GetNativeHandle(eThread->UniqueThread)) {