[vlc-devel] [PATCH] Avoid deadlock on stop() using UDP on Android

[Romain Vimont]

Android has no pthread_cancel(). Its behavior is emulated in
vlc_cancel() using a flag "killed", but it can lead to unavoidable race
conditions, typically between a call to vlc_object_alive() and a
blocking cond_wait().

This leads to reproducible (with an acceptable probability) deadlocks
when stopping just after playing UDP streams on Android:
https://mailman.videolan.org/pipermail/vlc-devel/2014-April/097861.html
https://mailman.videolan.org/pipermail/vlc-devel/2014-April/097912.html

Thus, we need to be able to wake up cond_wait() once the thread has to
finish. The commit 57eee64 did it, but it fails when block_FifoWake() is
called while the queue is not empty (in that case, it has no effect).
This is probably not a problem on platforms where pthread_cancel() is
available (the cond_wait() would be awakened), but this is a problem on
Android.

It seems the "b_force_wake" flag semantic could not be modified to match
a "finishing" state (e.g. block_FifoWake() is called in
input_DecoderWait()). Therefore, a separate flag "b_finishing" is added,
and set at the end of the UDP ThreadRead(), leading to set the "b_eof"
flag in the access_t.info structure.

With these changes, I cannot reproduce the deadlock at all.

Signed-off-by: Romain Vimont <rom at rom1v.com>
---
 include/vlc_block.h  |    2 ++
 modules/access/udp.c |   10 ++++++++--
 src/misc/block.c     |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/include/vlc_block.h b/include/vlc_block.h
index 75f98ff..264fd95 100644
--- a/include/vlc_block.h
+++ b/include/vlc_block.h
@@ -313,6 +313,8 @@ VLC_API void block_FifoPace( block_fifo_t *fifo, size_t max_depth, size_t max_si
 VLC_API void block_FifoEmpty( block_fifo_t * );
 VLC_API size_t block_FifoPut( block_fifo_t *, block_t * );
 VLC_API void block_FifoWake( block_fifo_t * );
+VLC_API void block_FifoFinish( block_fifo_t * );
+VLC_API bool block_FifoIsFinishing( block_fifo_t * );
 VLC_API block_t * block_FifoGet( block_fifo_t * ) VLC_USED;
 VLC_API block_t * block_FifoShow( block_fifo_t * );
 size_t block_FifoSize( const block_fifo_t *p_fifo ) VLC_USED;
diff --git a/modules/access/udp.c b/modules/access/udp.c
index 3c18cb8..29d5c53 100644
--- a/modules/access/udp.c
+++ b/modules/access/udp.c
@@ -231,7 +231,13 @@ static block_t *BlockUDP( access_t *p_access )
 {
     access_sys_t *sys = p_access->p_sys;
 
-    return block_FifoGet( sys->fifo );
+    block_t *block = block_FifoGet( sys->fifo );
+    if( unlikely( block_FifoIsFinishing( sys->fifo ) ) )
+    {
+        /* We should not read data anymore */
+        p_access->info.b_eof = true;
+    }
+    return block;
 }
 
 /*****************************************************************************
@@ -270,6 +276,6 @@ static void* ThreadRead( void *data )
         block_FifoPut( sys->fifo, pkt );
     }
 
-    block_FifoWake( sys->fifo );
+    block_FifoFinish( sys->fifo );
     return NULL;
 }
diff --git a/src/misc/block.c b/src/misc/block.c
index 3e953f1..fdfb89c 100644
--- a/src/misc/block.c
+++ b/src/misc/block.c
@@ -520,6 +520,7 @@ struct block_fifo_t
     size_t              i_depth;
     size_t              i_size;
     bool          b_force_wake;
+    bool                b_finishing;
 };
 
 block_fifo_t *block_FifoNew( void )
@@ -535,6 +536,7 @@ block_fifo_t *block_FifoNew( void )
     p_fifo->pp_last = &p_fifo->p_first;
     p_fifo->i_depth = p_fifo->i_size = 0;
     p_fifo->b_force_wake = false;
+    p_fifo->b_finishing = false;
 
     return p_fifo;
 }
@@ -647,6 +649,35 @@ void block_FifoWake( block_fifo_t *p_fifo )
 }
 
 /**
+ * Set the "finishing" flag, and force the queue to wake up.
+ * This flag is intented to avoid deadlocks.
+ *
+ * @param p_fifo the queue
+ */
+void block_FifoFinish( block_fifo_t *p_fifo )
+{
+    vlc_mutex_lock( &p_fifo->lock );
+    p_fifo->b_force_wake = true;
+    p_fifo->b_finishing = true;
+    vlc_cond_broadcast( &p_fifo->wait );
+    vlc_mutex_unlock( &p_fifo->lock );
+}
+
+/**
+ * Indicate whether the fifo is in "finishing" state.
+ *
+ * @param p_fifo the queue
+ */
+bool block_FifoIsFinishing( block_fifo_t *p_fifo )
+{
+    bool res;
+    vlc_mutex_lock( &p_fifo->lock );
+    res = p_fifo->b_finishing;
+    vlc_mutex_unlock( &p_fifo->lock );
+    return res;
+}
+
+/**
  * Dequeue the first block from the FIFO. If necessary, wait until there is
  * one block in the queue. This function is (always) cancellation point.
  *
@@ -667,6 +698,14 @@ block_t *block_FifoGet( block_fifo_t *p_fifo )
         vlc_cond_wait( &p_fifo->wait, &p_fifo->lock );
 
     vlc_cleanup_pop();
+
+    if( p_fifo->b_finishing )
+    {
+        /* The fifo is finishing, we should not handle new blocks */
+        vlc_mutex_unlock( &p_fifo->lock );
+        return NULL;
+    }
+
     b = p_fifo->p_first;
 
     p_fifo->b_force_wake = false;
-- 
1.7.10.4