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

@@ -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;
 };

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));

src/system/kernel/condition_variable.cpp

@@ -17,6 +17,10 @@
 #include <util/AutoLock.h>
 
 
+#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;

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);

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;