[Asterisk-code-review] app_confbridge: Fix bridge shutdown race condition. (asterisk[master])

[N A]

N A has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/18576 )


Change subject: app_confbridge: Fix bridge shutdown race condition.
......................................................................

app_confbridge: Fix bridge shutdown race condition.

A race condition exists where if a bridge is left vacant
for a split instant, the bridge will shut down and go
away at the same instant that somebody else could be trying
to join it. The newcomer will find the conference still
in conference_bridges and thus take it and run with it.
At the same time, shutdown of the bridge completes and it
is removed from conference_bridges.

As a result, the newcomer will end up joining the bridge
corresponding to the original conference that is now
defunct. When the next party joins the same named bridge,
it won't be found in conference_bridges and thus a new
bridge gets created. This can happen right afterwards,
but in theory it could happen at any point after the first.

As a result, the newcomer that joined the bridge during
the shutdown ends up stranded in a bridge that isn't
real and will never get conferenced with anything, since
it's been removed from the list of conferences already.

To prevent this, we now explicitly check during shutdown
and abort if the conference status is now empty. Additionally,
we set a flag to prevent newcomers from joining the old
bridge. On the join side, we ignore bridges that are shutting
down to ensure we don't join a dead bridge, and, importantly,
do not release the lock between the time the conference is
found and the conference state changes, to ensure that
an imminent shutdown will abort afterwards.

ASTERISK-30081 #close

Change-Id: I2a704dd7dd449bf008a1ee90768ddf95c28bae0d
---
M apps/app_confbridge.c
M apps/confbridge/include/confbridge.h
2 files changed, 34 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/76/18576/1

diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c
index ae17bca..f78cc3f 100644
--- a/apps/app_confbridge.c
+++ b/apps/app_confbridge.c
@@ -1524,6 +1524,16 @@
 {
 	struct pbx_find_info q = { .stacklen = 0 };
 
+	ao2_lock(conference);
+	if (conference->state == CONF_STATE_EMPTY) {
+		ast_debug(1, "Somebody joined the conference again before we could shut down, aborting shutdown\n");
+		ao2_unlock(conference);
+		return;
+	}
+	/* We are committed to shutting the bridge down for good. */
+	conference->shuttingdown = 1; /* Ensure nobody else tries to join, cause that's a dead end. */
+	ao2_unlock(conference);
+
 	/* Called with a reference to conference */
 	ao2_unlink(conference_bridges, conference);
 	send_conf_end_event(conference);
@@ -1739,6 +1749,25 @@
 
 	/* Attempt to find an existing conference bridge */
 	conference = ao2_find(conference_bridges, conference_name, OBJ_KEY);
+
+	/* If we found a bridge, ensure it isn't about to shut down. */
+	if (conference) {
+		ao2_lock(conference);
+		if (conference->shuttingdown) {
+			/* This is a dead end. Start fresh. */
+			ast_debug(1, "Conference is about to shut down, we'll make a new one\n");
+			ao2_unlock(conference);
+			conference = NULL;
+		}
+
+		/*
+		 * The above prevents us from entering a bridge that is shutting down, if the shutdown started first.
+		 * However, if we get here before the shutdown flag is set, we need to ensure the shutdown doesn't proceed.
+		 * That can only happen if when shutdown tries to proceed, it sees the bridge is active again.
+		 * Accordingly, do NOT release the conference lock until the state gets updated.
+		 */
+	}
+
 	if (conference && conference->b_profile.max_members) {
 		max_members_reached = conference->b_profile.max_members > conference->activeusers ? 0 : 1;
 	}
@@ -1864,7 +1893,11 @@
 			}
 		}
 
+		ao2_lock(conference);
+
 		ast_debug(1, "Created conference '%s' and linked to container.\n", conference_name);
+	} else {
+		ast_debug(1, "Reusing conference '%s'.\n", conference_name);
 	}
 
 	ao2_unlock(conference_bridges);
@@ -1872,8 +1905,6 @@
 	/* Setup conference bridge user parameters */
 	user->conference = conference;
 
-	ao2_lock(conference);
-
 	/* Determine if the new user should join the conference muted. */
 	if (ast_test_flag(&user->u_profile, USER_OPT_STARTMUTED)
 		|| (!ast_test_flag(&user->u_profile, USER_OPT_ADMIN) && conference->muted)) {
diff --git a/apps/confbridge/include/confbridge.h b/apps/confbridge/include/confbridge.h
index f90f185..6c0a538 100644
--- a/apps/confbridge/include/confbridge.h
+++ b/apps/confbridge/include/confbridge.h
@@ -252,6 +252,7 @@
 	unsigned int waitingusers;                                        /*!< Number of waiting users present */
 	unsigned int locked:1;                                            /*!< Is this conference bridge locked? */
 	unsigned int muted:1;                                             /*!< Is this conference bridge muted? */
+	unsigned int shuttingdown:1;                                      /*!< Is this conference committed to shutting down? */
 	struct ast_channel *playback_chan;                                /*!< Channel used for playback into the conference bridge */
 	struct ast_channel *record_chan;                                  /*!< Channel used for recording the conference */
 	struct ast_str *record_filename;                                  /*!< Recording filename. */

-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18576
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I2a704dd7dd449bf008a1ee90768ddf95c28bae0d
Gerrit-Change-Number: 18576
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220525/dcded761/attachment-0001.html>