output: protect audio_output.open with the mutex

There was a deadlock between the output thread and the player thread:
when the output thread failed (and closed itself) while the player
thread worked with the audio_output object, MPD could crash.

3 files changed, 33 insertions, 15 deletions

diff --git a/src/output_all.c b/src/output_all.c
index bf920206..2e7d3a72 100644
--- a/src/output_all.c
+++ b/src/output_all.c
@@ -169,6 +169,19 @@ static void audio_output_wait_all(void)
 		notify_wait(&audio_output_client_notify);
 }
 
+static void
+audio_output_reset_reopen(struct audio_output *ao)
+{
+	g_mutex_lock(ao->mutex);
+
+	if (!ao->open && ao->fail_timer != NULL) {
+		g_timer_destroy(ao->fail_timer);
+		ao->fail_timer = NULL;
+	}
+
+	g_mutex_unlock(ao->mutex);
+}
+
 /**
  * Resets the "reopen" flag on all audio devices.  MPD should
  * immediately retry to open the device instead of waiting for the
@@ -180,10 +193,7 @@ audio_output_all_reset_reopen(void)
 	for (unsigned i = 0; i < num_audio_outputs; ++i) {
 		struct audio_output *ao = &audio_outputs[i];
 
-		if (!ao->open && ao->fail_timer != NULL) {
-			g_timer_destroy(ao->fail_timer);
-			ao->fail_timer = NULL;
-		}
+		audio_output_reset_reopen(ao);
 	}
 }
 
@@ -289,6 +299,9 @@ static bool
 chunk_is_consumed_in(const struct audio_output *ao,
 		     const struct music_chunk *chunk)
 {
+	if (!ao->open)
+		return true;
+
 	if (ao->chunk == NULL)
 		return false;
 
@@ -312,9 +325,6 @@ chunk_is_consumed(const struct music_chunk *chunk)
 		const struct audio_output *ao = &audio_outputs[i];
 		bool consumed;
 
-		if (!ao->open)
-			continue;
-
 		g_mutex_lock(ao->mutex);
 		consumed = chunk_is_consumed_in(ao, chunk);
 		g_mutex_unlock(ao->mutex);
@@ -339,14 +349,15 @@ clear_tail_chunk(G_GNUC_UNUSED const struct music_chunk *chunk, bool *locked)
 	for (unsigned i = 0; i < num_audio_outputs; ++i) {
 		struct audio_output *ao = &audio_outputs[i];
 
-		locked[i] = ao->open;
-
-		if (!locked[i])
-			continue;
-
 		/* this mutex will be unlocked by the caller when it's
 		   ready */
 		g_mutex_lock(ao->mutex);
+		locked[i] = ao->open;
+
+		if (!locked[i]) {
+			g_mutex_unlock(ao->mutex);
+			continue;
+		}
 
 		assert(ao->chunk == chunk);
 		assert(ao->chunk_finished);
diff --git a/src/output_internal.h b/src/output_internal.h
index 3ec0485b..9fc2425c 100644
--- a/src/output_internal.h
+++ b/src/output_internal.h
@@ -65,6 +65,11 @@ struct audio_output {
 
 	/**
 	 * Is the device (already) open and functional?
+	 *
+	 * This attribute may only be modified by the output thread.
+	 * It is protected with #mutex: write accesses inside the
+	 * output thread and read accesses outside of it may only be
+	 * performed while the lock is held.
 	 */
 	bool open;
 
@@ -113,7 +118,7 @@ struct audio_output {
 	const struct music_pipe *pipe;
 
 	/**
-	 * This mutex protects #chunk and #chunk_finished.
+	 * This mutex protects #open, #chunk and #chunk_finished.
 	 */
 	GMutex *mutex;
 
diff --git a/src/output_thread.c b/src/output_thread.c
index b9325c07..62b89307 100644
--- a/src/output_thread.c
+++ b/src/output_thread.c
@@ -48,11 +48,11 @@ ao_close(struct audio_output *ao)
 
 	g_mutex_lock(ao->mutex);
 	ao->chunk = NULL;
+	ao->open = false;
 	g_mutex_unlock(ao->mutex);
 
 	ao_plugin_close(ao->plugin, ao->data);
 	pcm_convert_deinit(&ao->convert_state);
-	ao->open = false;
 
 	g_debug("closed plugin=%s name=\"%s\"", ao->plugin->name, ao->name);
 }
@@ -198,8 +198,10 @@ static gpointer audio_output_task(gpointer arg)
 			assert(!ao->open);
 			if (ret) {
 				pcm_convert_init(&ao->convert_state);
-				ao->open = true;
 
+				g_mutex_lock(ao->mutex);
+				ao->open = true;
+				g_mutex_unlock(ao->mutex);
 
 				g_debug("opened plugin=%s name=\"%s\" "
 					"audio_format=%u:%u:%u",