[Asterisk-code-review] app confbridge: Race between removing and playing name recor... (asterisk[master])

[Robert Mordec]

Robert Mordec has uploaded a new change for review. ( https://gerrit.asterisk.org/5685 )

Change subject: app_confbridge: Race between removing and playing name recording while leaving
......................................................................

app_confbridge: Race between removing and playing name recording while leaving

When user leaves a conference, its channel calls async_play_sound_file()
in order to play the name announcement and then unlinks the sound file.
The async_play_sound_file() function adds a task to conference playback queue,
which then runs playback_common() function in a different thread.

It leads to a race condition when, in some cases, channel thread may unlink
the sound file before playback_common() had a chance to open it.

This patch creates a file deletion task, that is queued after playback.

ASTERISK-27012 #close

Change-Id: I412f7922d412004b80917d4e892546c15bd70dd3
---
M apps/app_confbridge.c
1 file changed, 75 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/85/5685/1

diff --git a/apps/app_confbridge.c b/apps/app_confbridge.c
index a9f917b..42a45c1 100644
--- a/apps/app_confbridge.c
+++ b/apps/app_confbridge.c
@@ -2149,6 +2149,80 @@
 	return 0;
 }
 
+struct async_delete_name_rec_task_data {
+	struct confbridge_conference *conference;
+	char filename[0];
+};
+
+static struct async_delete_name_rec_task_data *async_delete_name_rec_task_data_alloc(
+	struct confbridge_conference *conference, const char *filename)
+{
+	struct async_delete_name_rec_task_data *atd;
+
+	atd = ast_malloc(sizeof(*atd) + strlen(filename) + 1);
+	if (!atd) {
+		return NULL;
+	}
+
+	/* Safe */
+	strcpy(atd->filename, filename);
+	atd->conference = conference;
+
+	return atd;
+}
+
+static void async_delete_name_rec_task_data_destroy(struct async_delete_name_rec_task_data *atd)
+{
+	ast_free(atd);
+}
+
+/*!
+ * \brief Delete user's name file asynchronously
+ *
+ * This runs in the playback queue taskprocessor. This ensures that
+ * sound file is removed after playback is finished and not before.
+ *
+ * \param data An async_delete_name_rec_task_data
+ * \return 0
+ */
+static int async_delete_name_rec_task(void *data)
+{
+	struct async_delete_name_rec_task_data *atd = data;
+
+	ast_filedelete(atd->filename, NULL);
+	ast_log(LOG_DEBUG, "Conference '%s' removed user name file '%s'\n",
+		atd->conference->name, atd->filename);
+
+	async_delete_name_rec_task_data_destroy(atd);
+	return 0;
+}
+
+static int async_delete_name_rec(struct confbridge_conference *conference,
+	const char *filename)
+{
+	struct async_delete_name_rec_task_data *atd;
+
+	if (ast_strlen_zero(filename)) {
+		return 0;
+	} else if (!sound_file_exists(filename)) {
+		return 0;
+	}
+
+	atd = async_delete_name_rec_task_data_alloc(conference, filename);
+	if (!atd) {
+		return -1;
+	}
+
+	if (ast_taskprocessor_push(conference->playback_queue, async_delete_name_rec_task, atd)) {
+		ast_log(LOG_WARNING, "Conference '%s' was unable to remove user name file '%s'\n",
+			conference->name, filename);
+		async_delete_name_rec_task_data_destroy(atd);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int join_callback(struct ast_bridge_channel *bridge_channel, void *ignore)
 {
 	async_play_sound_ready(bridge_channel->chan);
@@ -2404,6 +2478,7 @@
 		async_play_sound_file(conference, user.name_rec_location, NULL);
 		async_play_sound_file(conference,
 			conf_get_sound(CONF_SOUND_HAS_LEFT, conference->b_profile.sounds), NULL);
+		async_delete_name_rec(conference, user.name_rec_location);
 	}
 
 	/* play the leave sound */
@@ -2429,10 +2504,6 @@
 	}
 	if (volume_adjustments[1]) {
 		ast_audiohook_volume_set(chan, AST_AUDIOHOOK_DIRECTION_WRITE, volume_adjustments[1]);
-	}
-
-	if (!ast_strlen_zero(user.name_rec_location)) {
-		ast_filedelete(user.name_rec_location, NULL);
 	}
 
 confbridge_cleanup:

-- 
To view, visit https://gerrit.asterisk.org/5685
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I412f7922d412004b80917d4e892546c15bd70dd3
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Robert Mordec <r.mordec at slican.pl>