From fbe0c27a94fff9dff7c42e12ee96f789b314c746 Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Sat, 17 May 2008 10:21:37 +0000 Subject: [PATCH] axeld + bonefish: Changed condition variables so that it is allowed to block (e.g. lock mutexes etc.) between Add() and Wait(). This fixes #2059, since the block writer used them this way and could thusly fail to wait for a condition variable, causing a temporary stack object to be used past its lifetime. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@25525 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- headers/private/kernel/condition_variable.h | 6 +-- src/add-ons/kernel/drivers/tty/tty.cpp | 6 ++- src/system/kernel/condition_variable.cpp | 45 +++++++++++++++------ src/system/kernel/fs/fifo.cpp | 4 +- src/system/kernel/team.cpp | 4 +- 5 files changed, 43 insertions(+), 22 deletions(-) diff --git a/headers/private/kernel/condition_variable.h b/headers/private/kernel/condition_variable.h index 7f22e41f7f..5706c91357 100644 --- a/headers/private/kernel/condition_variable.h +++ b/headers/private/kernel/condition_variable.h @@ -27,9 +27,8 @@ public: inline ~ConditionVariableEntry(); #endif - bool Add(const void* object, uint32 flags = 0); - status_t Wait(uint32 timeoutFlags = 0, - bigtime_t timeout = 0); + bool Add(const void* object); + status_t Wait(uint32 flags = 0, bigtime_t timeout = 0); status_t Wait(const void* object, uint32 flags = 0, bigtime_t timeout = 0); @@ -42,6 +41,7 @@ private: private: ConditionVariable* fVariable; struct thread* fThread; + status_t fWaitStatus; friend class ConditionVariable; }; diff --git a/src/add-ons/kernel/drivers/tty/tty.cpp b/src/add-ons/kernel/drivers/tty/tty.cpp index bad835d8fc..551620d539 100644 --- a/src/add-ons/kernel/drivers/tty/tty.cpp +++ b/src/add-ons/kernel/drivers/tty/tty.cpp @@ -414,14 +414,16 @@ RequestOwner::Wait(bool interruptable, bigtime_t timeout) // add an entry to wait on ConditionVariableEntry entry; - entry.Add(this, interruptable ? B_CAN_INTERRUPT : 0); + entry.Add(this); locker.Unlock(); // wait TRACE(("%p->RequestOwner::Wait(): waiting for condition...\n", this)); - error = entry.Wait(B_RELATIVE_TIMEOUT, timeout); + error = entry.Wait( + (interruptable ? B_CAN_INTERRUPT : 0) | B_RELATIVE_TIMEOUT, + timeout); TRACE(("%p->RequestOwner::Wait(): condition occurred: %lx\n", this, error)); diff --git a/src/system/kernel/condition_variable.cpp b/src/system/kernel/condition_variable.cpp index 6e95aea18e..6fec06b534 100644 --- a/src/system/kernel/condition_variable.cpp +++ b/src/system/kernel/condition_variable.cpp @@ -17,6 +17,10 @@ #include +#define STATUS_ADDED 1 +#define STATUS_WAITING 2 + + static const int kConditionVariableHashSize = 512; @@ -90,7 +94,7 @@ dump_condition_variable(int argc, char** argv) bool -ConditionVariableEntry::Add(const void* object, uint32 flags) +ConditionVariableEntry::Add(const void* object) { ASSERT(object != NULL); @@ -100,16 +104,12 @@ ConditionVariableEntry::Add(const void* object, uint32 flags) fVariable = sConditionVariableHash.Lookup(object); - thread_prepare_to_block(fThread, flags, - THREAD_BLOCK_TYPE_CONDITION_VARIABLE, fVariable); - if (fVariable == NULL) { - SpinLocker threadLocker(thread_spinlock); - thread_unblock_locked(fThread, B_ENTRY_NOT_FOUND); + fWaitStatus = B_ENTRY_NOT_FOUND; return false; } - // add to the queue for the variable + fWaitStatus = STATUS_ADDED; fVariable->fEntries.Add(this); return true; @@ -117,7 +117,7 @@ ConditionVariableEntry::Add(const void* object, uint32 flags) status_t -ConditionVariableEntry::Wait(uint32 timeoutFlags, bigtime_t timeout) +ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) { if (!are_interrupts_enabled()) { panic("wait_for_condition_variable_entry() called with interrupts " @@ -127,15 +127,28 @@ ConditionVariableEntry::Wait(uint32 timeoutFlags, bigtime_t timeout) InterruptsLocker _; + SpinLocker conditionLocker(sConditionVariablesLock); + + if (fVariable == NULL) + return fWaitStatus; + + thread_prepare_to_block(fThread, flags, + THREAD_BLOCK_TYPE_CONDITION_VARIABLE, fVariable); + + fWaitStatus = STATUS_WAITING; + + conditionLocker.Unlock(); + SpinLocker threadLocker(thread_spinlock); + status_t error; - if ((timeoutFlags & (B_RELATIVE_TIMEOUT | B_ABSOLUTE_TIMEOUT)) != 0) - error = thread_block_with_timeout_locked(timeoutFlags, timeout); + if ((flags & (B_RELATIVE_TIMEOUT | B_ABSOLUTE_TIMEOUT)) != 0) + error = thread_block_with_timeout_locked(flags, timeout); else error = thread_block_locked(thread_get_current_thread()); threadLocker.Unlock(); - SpinLocker locker(sConditionVariablesLock); + conditionLocker.Lock(); // remove entry from variable, if not done yet if (fVariable != NULL) { @@ -151,7 +164,7 @@ status_t ConditionVariableEntry::Wait(const void* object, uint32 flags, bigtime_t timeout) { - if (Add(object, flags)) + if (Add(object)) return Wait(flags, timeout); return B_ENTRY_NOT_FOUND; } @@ -294,7 +307,13 @@ ConditionVariable::_NotifyChecked(bool all, status_t result) while (ConditionVariableEntry* entry = fEntries.RemoveHead()) { entry->fVariable = NULL; - thread_unblock_locked(entry->fThread, B_OK); + if (entry->fWaitStatus <= 0) + continue; + + if (entry->fWaitStatus == STATUS_WAITING) + thread_unblock_locked(entry->fThread, result); + + entry->fWaitStatus = result; if (!all) break; diff --git a/src/system/kernel/fs/fifo.cpp b/src/system/kernel/fs/fifo.cpp index 4c2ea47209..1130d10f33 100644 --- a/src/system/kernel/fs/fifo.cpp +++ b/src/system/kernel/fs/fifo.cpp @@ -358,13 +358,13 @@ Inode::WriteDataToBuffer(const void *_data, size_t *_length, bool nonBlocking) return B_WOULD_BLOCK; ConditionVariableEntry entry; - entry.Add(this, B_CAN_INTERRUPT); + entry.Add(this); WriteRequest request(minToWrite); fWriteRequests.Add(&request); mutex_unlock(&fRequestLock); - status_t status = entry.Wait(); + status_t status = entry.Wait(B_CAN_INTERRUPT); mutex_lock(&fRequestLock); fWriteRequests.Remove(&request); diff --git a/src/system/kernel/team.cpp b/src/system/kernel/team.cpp index a4d7d6ac5c..a0c1971803 100644 --- a/src/system/kernel/team.cpp +++ b/src/system/kernel/team.cpp @@ -1867,7 +1867,7 @@ wait_for_child(pid_t child, uint32 flags, int32 *_reason, ConditionVariableEntry deadWaitEntry; if (status == B_WOULD_BLOCK && (flags & WNOHANG) == 0) - deadWaitEntry.Add(team->dead_children, B_CAN_INTERRUPT); + deadWaitEntry.Add(team->dead_children); locker.Unlock(); @@ -1888,7 +1888,7 @@ wait_for_child(pid_t child, uint32 flags, int32 *_reason, return status; } - status = deadWaitEntry.Wait(); + status = deadWaitEntry.Wait(B_CAN_INTERRUPT); if (status == B_INTERRUPTED) { T(WaitForChildDone(status)); return status;