Fix race condition when accessing last_frame via get_last_frame()

_x_get_current_frame_data() called get_last_frame() and locked the returned
frame afterwards. At the same time, video_out_loop() unlocked last_frame to
assign a different img afterwards. So in case the image got unlocked before
it gets locked again, the image resides already on the free image queue. So
when the image gets unlocked, it will be put a second time to the queue and
hence cause a loop in the list the queue is based on. Getting an image from
the queue will then run endlessly.
To fix this issue, a new mutex is introduced which protects write access to
last_frame and read accesses via get_last_frame() from other threads. Next,
the semantic of get_last_frame() had to be changed to return a locked image
already. Finally, functions calling get_last_frame() had to be adapted to
its new behavior (there was only a single function in xine-lib which had to
be adapted: _x_get_current_frame_data()).

2 files changed, 21 insertions, 3 deletions

diff --git a/src/xine-engine/video_out.c b/src/xine-engine/video_out.c
index 2bfc0efef..bceb38a58 100644
--- a/src/xine-engine/video_out.c
+++ b/src/xine-engine/video_out.c
@@ -91,6 +91,7 @@ typedef struct {
   img_buf_fifo_t           *free_img_buf_queue;
   img_buf_fifo_t           *display_img_buf_queue;
 
+  pthread_mutex_t           last_frame_mutex;
   vo_frame_t               *last_frame;
   vo_frame_t               *img_backup;
 
@@ -1038,12 +1039,16 @@ static void overlay_and_display_frame (vos_t *this,
   }
 
   /* hold current frame for snapshot feature */
+  pthread_mutex_lock(&this->last_frame_mutex);
+
   if( this->last_frame ) {
     vo_frame_dec_lock( this->last_frame );
   }
   vo_frame_inc_lock( img );
   this->last_frame = img;
 
+  pthread_mutex_unlock(&this->last_frame_mutex);
+
   this->driver->display_frame (this->driver, img);
 
   /*
@@ -1325,8 +1330,10 @@ static void *video_out_loop (void *this_gen) {
     this->img_backup = NULL;
   }
   if (this->last_frame) {
+    pthread_mutex_lock(&this->last_frame_mutex);
     vo_frame_dec_lock( this->last_frame );
     this->last_frame = NULL;
+    pthread_mutex_unlock(&this->last_frame_mutex);
   }
 
   return NULL;
@@ -1698,6 +1705,8 @@ static void vo_exit (xine_video_port_t *this_gen) {
   free (this->free_img_buf_queue);
   free (this->display_img_buf_queue);
 
+  pthread_mutex_destroy(&this->last_frame_mutex);
+
   pthread_cond_destroy(&this->trigger_drawing_cond);
   pthread_mutex_destroy(&this->trigger_drawing_mutex);
 
@@ -1706,7 +1715,17 @@ static void vo_exit (xine_video_port_t *this_gen) {
 
 static vo_frame_t *vo_get_last_frame (xine_video_port_t *this_gen) {
   vos_t      *this = (vos_t *) this_gen;
-  return this->last_frame;
+  vo_frame_t *last_frame;
+  
+  pthread_mutex_lock(&this->last_frame_mutex);
+
+  last_frame = this->last_frame;
+  if (last_frame)
+    vo_frame_inc_lock(last_frame);
+
+  pthread_mutex_unlock(&this->last_frame_mutex);
+
+  return last_frame;
 }
 
 /*
@@ -1887,6 +1906,7 @@ xine_video_port_t *_x_vo_new_port (xine_t *xine, vo_driver_t *driver, int grabon
   this->display_img_buf_queue = vo_new_img_buf_queue ();
   this->video_loop_running    = 0;
 
+  pthread_mutex_init(&this->last_frame_mutex, NULL);
   this->last_frame            = NULL;
   this->img_backup            = NULL;
 
diff --git a/src/xine-engine/xine.c b/src/xine-engine/xine.c
index a03ac5686..73cd9ae7e 100644
--- a/src/xine-engine/xine.c
+++ b/src/xine-engine/xine.c
@@ -2028,8 +2028,6 @@ static int _x_get_current_frame_data (xine_stream_t *stream,
 
   stream->xine->port_ticket->acquire(stream->xine->port_ticket, 0);
   frame = stream->video_out->get_last_frame (stream->video_out);
-  if (frame)
-    frame->lock(frame);
   stream->xine->port_ticket->release(stream->xine->port_ticket, 0);
 
   if (!frame) {