From 33dd436f25ae85ca0ac71fcada152a9ad15234af Mon Sep 17 00:00:00 2001
From: Augustin Cavalier <waddlesplash@gmail.com>
Date: Sat, 8 Apr 2023 15:10:35 -0400
Subject: [PATCH] kernel/team: Fix race condition in team loading wait.

The condition variable that the load_image'ing thread is waiting on
is also owned by that same thread, so as soon as it wakes up, it
will soon return, thus destroying it. Under high load or other unlucky
scheduling conditions, it seems this could occur before the other thread
had even returned from the condition variable's NotifyAll.

Since team->loading_info is protected by the team lock, simply
acquire the team lock once more after being awoken and returning,
to synchronize and prevent this race.

Should fix #18352.
---
 src/system/kernel/image.cpp | 16 +++++++---------
 src/system/kernel/team.cpp  | 20 ++++++++++++++------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/system/kernel/image.cpp b/src/system/kernel/image.cpp
index 9f3b9b3085..c3bad82ab1 100644
--- a/src/system/kernel/image.cpp
+++ b/src/system/kernel/image.cpp
@@ -426,20 +426,18 @@ notify_loading_app(status_t result, bool suspend)
 
 	TeamLocker teamLocker(team);
 
-	if (team->loading_info) {
+	if (team->loading_info != NULL) {
 		// there's indeed someone waiting
-		struct team_loading_info* loadingInfo = team->loading_info;
-		team->loading_info = NULL;
-
-		loadingInfo->result = result;
-
-		// we're done with the team stuff, get the scheduler lock instead
-		teamLocker.Unlock();
 
 		thread_prepare_suspend();
 
 		// wake up the waiting thread
-		loadingInfo->condition.NotifyAll();
+		team->loading_info->result = result;
+		team->loading_info->condition.NotifyAll();
+		team->loading_info = NULL;
+
+		// we're done with the team stuff
+		teamLocker.Unlock();
 
 		// suspend ourselves, if desired
 		if (suspend)
diff --git a/src/system/kernel/team.cpp b/src/system/kernel/team.cpp
index 356563a2c8..c08c284a28 100644
--- a/src/system/kernel/team.cpp
+++ b/src/system/kernel/team.cpp
@@ -1765,11 +1765,13 @@ load_image_internal(char**& _flatArgs, size_t flatArgsSize, int32 argCount,
 		return B_NO_MEMORY;
 	BReference<Team> teamReference(team, true);
 
+	BReference<Team> teamLoadingReference;
 	if ((flags & B_WAIT_TILL_LOADED) != 0) {
 		loadingInfo.condition.Init(team, "image load");
 		loadingInfo.condition.Add(&loadingWaitEntry);
 		loadingInfo.result = B_ERROR;
 		team->loading_info = &loadingInfo;
+		teamLoadingReference = teamReference;
 	}
 
 	// get the parent team
@@ -1892,6 +1894,14 @@ load_image_internal(char**& _flatArgs, size_t flatArgsSize, int32 argCount,
 		// responsible for unsetting `loading_info` in the team structure.
 		loadingWaitEntry.Wait();
 
+		// We must synchronize with the thread that woke us up, to ensure
+		// there are no remaining consumers of the team_loading_info.
+		team->Lock();
+		if (team->loading_info != NULL)
+			panic("team loading wait complete, but loading_info != NULL");
+		team->Unlock();
+		teamLoadingReference.Unset();
+
 		if (loadingInfo.result < B_OK)
 			return loadingInfo.result;
 	}
@@ -3288,15 +3298,13 @@ team_delete_team(Team* team, port_id debuggerPort)
 
 	TeamLocker teamLocker(team);
 
-	if (team->loading_info) {
+	if (team->loading_info != NULL) {
 		// there's indeed someone waiting
-		struct team_loading_info* loadingInfo = team->loading_info;
-		team->loading_info = NULL;
-
-		loadingInfo->result = B_ERROR;
+		team->loading_info->result = B_ERROR;
 
 		// wake up the waiting thread
-		loadingInfo->condition.NotifyAll();
+		team->loading_info->condition.NotifyAll();
+		team->loading_info = NULL;
 	}
 
 	// notify team watchers