[pulseaudio-discuss] [PATCH 3/4] sink, source: remove some assumptions about stream "attached" state

[Tanu Kaskinen]

Streams are detached when they are removed or moved away from a device,
or when a filter device that they're connected to is removed or moved.
If these cases overlap, a crash will happen due to "double-detaching".
This can happen if a filter sink is removed, and a stream connected to
that filter sink removes itself when its sink goes away.

Here are the steps in more detail: When a filter sink is unloaded, first
it will unlink its own sink input. This will cause the filter sink's
input to be detached. The filter sink propagates the detachment to all
inputs connected to it using pa_sink_detach_within_thread(). After the
filter sink is done unlinking its own sink input, it will unlink the
sink. This will cause at least module-combine-sink to remove its sink
input if it had one connected to the removed filter sink. When the
combine sink removes its sink input, that input will get detached again,
and a crash follows.

We can relax the assertions a bit, and skip the detach() call if the
sink input is already detached.

I think a better fix would be to unlink the sink before the sink input
when unloading a filter sink - that way we could avoid the
double-detaching - but that would be a much more complicated change. I
decided to go with this simple fix for now.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=98617
---
 src/pulsecore/sink.c   | 27 +++++++++++++++------------
 src/pulsecore/source.c | 18 ++++++++++--------
 2 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 64f2ef7..6990b80 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -2533,11 +2533,12 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
              * sink input handling a few lines down at
              * PA_SINK_MESSAGE_START_MOVE, too. */
 
-            pa_assert(i->thread_info.attached);
-            i->thread_info.attached = false;
+            if (i->thread_info.attached) {
+                i->thread_info.attached = false;
 
-            if (i->detach)
-                i->detach(i);
+                if (i->detach)
+                    i->detach(i);
+            }
 
             pa_sink_input_set_state_within_thread(i, i->state);
 
@@ -2632,11 +2633,12 @@ int pa_sink_process_msg(pa_msgobject *o, int code, void *userdata, int64_t offse
                 }
             }
 
-            pa_assert(i->thread_info.attached);
-            i->thread_info.attached = false;
+            if (i->thread_info.attached) {
+                i->thread_info.attached = false;
 
-            if (i->detach)
-                i->detach(i);
+                if (i->detach)
+                    i->detach(i);
+            }
 
             /* Let's remove the sink input ...*/
             pa_hashmap_remove_and_free(s->thread_info.inputs, PA_UINT32_TO_PTR(i->index));
@@ -2929,11 +2931,12 @@ void pa_sink_detach_within_thread(pa_sink *s) {
     pa_assert(PA_SINK_IS_LINKED(s->thread_info.state));
 
     PA_HASHMAP_FOREACH(i, s->thread_info.inputs, state) {
-        pa_assert(i->thread_info.attached);
-        i->thread_info.attached = false;
+        if (i->thread_info.attached) {
+            i->thread_info.attached = false;
 
-        if (i->detach)
-            i->detach(i);
+            if (i->detach)
+                i->detach(i);
+        }
     }
 
     if (s->monitor_source)
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 44bdad8..f1afc0b 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -2059,11 +2059,12 @@ int pa_source_process_msg(pa_msgobject *object, int code, void *userdata, int64_
 
             pa_source_output_set_state_within_thread(o, o->state);
 
-            pa_assert(o->thread_info.attached);
-            o->thread_info.attached = false;
+            if (o->thread_info.attached) {
+                o->thread_info.attached = false;
 
-            if (o->detach)
-                o->detach(o);
+                if (o->detach)
+                    o->detach(o);
+            }
 
             if (o->thread_info.direct_on_input) {
                 pa_hashmap_remove(o->thread_info.direct_on_input->thread_info.direct_outputs, PA_UINT32_TO_PTR(o->index));
@@ -2286,11 +2287,12 @@ void pa_source_detach_within_thread(pa_source *s) {
     pa_assert(PA_SOURCE_IS_LINKED(s->thread_info.state));
 
     PA_HASHMAP_FOREACH(o, s->thread_info.outputs, state) {
-        pa_assert(o->thread_info.attached);
-        o->thread_info.attached = false;
+        if (o->thread_info.attached) {
+            o->thread_info.attached = false;
 
-        if (o->detach)
-            o->detach(o);
+            if (o->detach)
+                o->detach(o);
+        }
     }
 }
 
-- 
2.10.2