[pulseaudio-discuss] [PATCH 6/6] virtual-sink: Fix a crash when moving the sink to a new master right after setup.

[Tanu Kaskinen]

If the virtual sink is moved to a new master right after it has been created,
then the virtual sink input's memblockq can be rewound to a negative read
index. The data written prior to the move starts from index zero, so after the
rewind there's a bit of silence. If the memblockq doesn't have a silence
memchunk set, then pa_memblockq_peek() will return zero in such case, and the
returned memchunk's memblock pointer will be NULL.

That scenario wasn't taken into account in the implementation of
sink_input_pop_cb. Setting a silence memchunk for the memblockq solves this
problem, because pa_memblock_peek() will now return a valid memblock if the
read index happens to point to a hole in the memblockq.

I believe this isn't the best possible solution, though. It doesn't really make
sense to rewind the sink input's memblockq beyond index 0 in the first place,
because now when the stream starts to play to the new master sink, there's some
unnecessary silence before the actual data starts. This is a small problem,
though, and I don't grok the rewinding system well enough to know how to fix
this issue properly.

I went through all files that call pa_memblockq_peek() to see if there are more
similar bugs. play-memblockq.c was the only one that looked to me like it might
be broken in the same way. I didn't try reproducing the bug with
play-memblockq.c, though, so I just added a FIXME comment there.
---
 src/modules/module-virtual-sink.c |   10 +++++-----
 src/modules/rtp/rtp.h             |    3 +++
 src/pulsecore/play-memblockq.c    |    6 ++++++
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-sink.c
index bad159c..58ea932 100644
--- a/src/modules/module-virtual-sink.c
+++ b/src/modules/module-virtual-sink.c
@@ -483,6 +483,7 @@ int pa__init(pa_module*m) {
     pa_sink_new_data sink_data;
     pa_bool_t use_volume_sharing = FALSE;
     pa_bool_t force_flat_volume = FALSE;
+    pa_memchunk silence;
 
     pa_assert(m);
 
@@ -606,12 +607,11 @@ int pa__init(pa_module*m) {
 
     u->sink->input_to_master = u->sink_input;
 
-    /* (9) IF YOU REQUIRE A FIXED BLOCK SIZE MAKE SURE TO PASS A
-     * SILENCE MEMBLOCK AS LAST PARAMETER
-     * HERE. pa_sink_input_get_silence() IS USEFUL HERE. */
-    u->memblockq = pa_memblockq_new(0, MEMBLOCKQ_MAXLENGTH, 0, pa_frame_size(&ss), 1, 1, 0, NULL);
+    pa_sink_input_get_silence(u->sink_input, &silence);
+    u->memblockq = pa_memblockq_new(0, MEMBLOCKQ_MAXLENGTH, 0, pa_frame_size(&ss), 1, 1, 0, &silence);
+    pa_memblock_unref(silence.memblock);
 
-    /* (10) INITIALIZE ANYTHING ELSE YOU NEED HERE */
+    /* (9) INITIALIZE ANYTHING ELSE YOU NEED HERE */
 
     pa_sink_put(u->sink);
     pa_sink_input_put(u->sink_input);
diff --git a/src/modules/rtp/rtp.h b/src/modules/rtp/rtp.h
index b197e82..e975e75 100644
--- a/src/modules/rtp/rtp.h
+++ b/src/modules/rtp/rtp.h
@@ -40,6 +40,9 @@ typedef struct pa_rtp_context {
 } pa_rtp_context;
 
 pa_rtp_context* pa_rtp_context_init_send(pa_rtp_context *c, int fd, uint32_t ssrc, uint8_t payload, size_t frame_size);
+
+/* If the memblockq doesn't have a silence memchunk set, then the caller must
+ * guarantee that the current read index doesn't point to a hole. */
 int pa_rtp_send(pa_rtp_context *c, size_t size, pa_memblockq *q);
 
 pa_rtp_context* pa_rtp_context_init_recv(pa_rtp_context *c, int fd, size_t frame_size);
diff --git a/src/pulsecore/play-memblockq.c b/src/pulsecore/play-memblockq.c
index 0d6da3e..f075a5b 100644
--- a/src/pulsecore/play-memblockq.c
+++ b/src/pulsecore/play-memblockq.c
@@ -135,6 +135,12 @@ static int sink_input_pop_cb(pa_sink_input *i, size_t nbytes, pa_memchunk *chunk
         return -1;
     }
 
+    /* FIXME: u->memblockq doesn't have a silence memchunk set, so
+     * pa_memblockq_peek() will return 0 without returning any memblock if the
+     * read index points to a hole. If the memblockq is rewound beyond index 0,
+     * then there will be a hole. */
+    pa_assert(chunk->memblock);
+
     chunk->length = PA_MIN(chunk->length, nbytes);
     pa_memblockq_drop(u->memblockq, chunk->length);
 
-- 
1.7.3.4