[pulseaudio-discuss] [PATCH 05/15] protocol-native: Protect stream unlinking against recursion

[Tanu Kaskinen]

It's possible (at some point in the future if not with the current
code base) that calling pa_source_output_unlink() from
record_stream_unlink() in some situation causes a recursive call to
record_stream_unlink(). If that happens, then we must ensure that
record_stream_unlink() exits early, so that we don't unref the source
output twice. We can achieve that by setting s->connection to NULL as
soon as possible. (And the same goes for playback streams.)

I ran into this issue when I was adding edge objects to the system.
When a stream ended and it was unlinked, the edge that was associated
with the stream got deleted, and as part of the edge deletion process
the currently-being-unlinked stream was killed, causing a recursive
unlink call, and a crash followed. From debugging this I learned a
couple of patterns to make the code more robust: return early from
unlink functions if recursion happens, and don't call the kill()
callback for streams that are already being unlinked. (There will be a
couple more patches implementing these safeguards.)
---
 src/pulsecore/protocol-native.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
index 3434d2b..0254093 100644
--- a/src/pulsecore/protocol-native.c
+++ b/src/pulsecore/protocol-native.c
@@ -474,14 +474,15 @@ static void record_stream_unlink(record_stream *s) {
     if (!s->connection)
         return;
 
+    pa_assert_se(pa_idxset_remove_by_data(s->connection->record_streams, s, NULL) == s);
+    s->connection = NULL;
+
     if (s->source_output) {
         pa_source_output_unlink(s->source_output);
         pa_source_output_unref(s->source_output);
         s->source_output = NULL;
     }
 
-    pa_assert_se(pa_idxset_remove_by_data(s->connection->record_streams, s, NULL) == s);
-    s->connection = NULL;
     record_stream_unref(s);
 }
 
@@ -768,17 +769,18 @@ static void playback_stream_unlink(playback_stream *s) {
     if (!s->connection)
         return;
 
+    if (s->drain_request)
+        pa_pstream_send_error(s->connection->pstream, s->drain_tag, PA_ERR_NOENTITY);
+
+    pa_assert_se(pa_idxset_remove_by_data(s->connection->output_streams, s, NULL) == s);
+    s->connection = NULL;
+
     if (s->sink_input) {
         pa_sink_input_unlink(s->sink_input);
         pa_sink_input_unref(s->sink_input);
         s->sink_input = NULL;
     }
 
-    if (s->drain_request)
-        pa_pstream_send_error(s->connection->pstream, s->drain_tag, PA_ERR_NOENTITY);
-
-    pa_assert_se(pa_idxset_remove_by_data(s->connection->output_streams, s, NULL) == s);
-    s->connection = NULL;
     playback_stream_unref(s);
 }
 
-- 
1.8.3.1