GNOME Bugzilla – Bug 777146
vaapisink: segfault caused by race condition with OverlayInterface expose method
Last modified: 2017-01-20 15:24:44 UTC
gst_vaapisink_overlay_expose calls gst_vaapisink_show_frame, passing a pointer to sink->video_buffer, but that variable is accessed outside any lock. Meanwhile, gst_vaapisink_show_frame, which is also called from the main data-flow thread, explicitly clears that variable outside the lock. The result is that application calls to the expose method can occasionally cause a segfault during playback.
Created attachment 343315 [details] [review] Add mutex protection around video_buffer member variable
Review of attachment 343315 [details] [review]: Good catch! thanks! Just a couple comments. ::: gst/vaapi/gstvaapisink.c @@ +578,3 @@ GstVaapiSink *const sink = GST_VAAPISINK (overlay); + gst_vaapisink_reconfigure_window (sink); I wonder if this would launch unneeded window configures. What do you think about this approach? gst_vaapi_display_lock (GST_VAAPI_PLUGIN_BASE_DISPLAY (sink)); if (sink->video_buffer) buffer = gst_buffer_ref (sink->video_buffer); gst_vaapi_display_unlock (GST_VAAPI_PLUGIN_BASE_DISPLAY (sink)); if (buffer) { gst_vaapisink_reconfigure_window (sink); gst_vaapisink_show_frame (GST_VIDEO_SINK_CAST (sink), buffer); } @@ +1463,3 @@ + if (oldBuffer != NULL) { + gst_buffer_unref (oldBuffer); + } Is this truly required?
Review of attachment 343315 [details] [review]: ::: gst/vaapi/gstvaapisink.c @@ +578,3 @@ GstVaapiSink *const sink = GST_VAAPISINK (overlay); + gst_vaapisink_reconfigure_window (sink); I thought about that, but the reason I moved the call to reconfigure_window outside of the check on sink->video_buffer, is that its implementation doesn't seem to have anything to do with the buffer. It only works on the caps, which could get set/changed independent of the buffer (or even before the first buffer arrives). Also, by reffing sink->video_buffer inside the lock, then releasing the lock, and then calling show_frame, there's the possibility of a new frame arriving in the interim. I think that would cause this method to re-show the previous frame out of sequence. @@ +1463,3 @@ + if (oldBuffer != NULL) { + gst_buffer_unref (oldBuffer); + } Do you mean the check for whether oldBuffer is null? I didn't know whether gst_buffer_unref was robust to null pointers, so maybe that's not necessary. Otherwise, yes, it's necessary to keep the assignment of the sink->video_buffer variable inside the mutex lock in order to avoid a race with calls to the expose method on separate threads. The actual deadlock possibility, for which we need to leave the lock, appears to only be related to the gst_buffer_unref operation.
Created attachment 343388 [details] [review] vaapisink: don't use member variable outside lock Thus a race condition segfault is avoided.
(In reply to Víctor Manuel Jáquez Leal from comment #5) > Created attachment 343388 [details] [review] [review] > vaapisink: don't use member variable outside lock > > Thus a race condition segfault is avoided. @Matt, What do you think about this version of the patch? I think it is simpler. But I haven't tested it.
I like the general approach. It certainly simplifies the overlay_expose method. However, the race condition is still there as long as the call, "gst_buffer_replace(&sink->videoBuffer, buffer);" is made outside the lock.
(In reply to Matt Staples from comment #7) > I like the general approach. It certainly simplifies the overlay_expose > method. > However, the race condition is still there as long as the call, > "gst_buffer_replace(&sink->videoBuffer, buffer);" is made outside the lock. Got it! I'll update the patch. Thanks!
Created attachment 343453 [details] [review] vaapisink: don't use member variable outside lock Thus a race condition segfault is avoided. Original-patch-by: Matt Staples <staples255@gmail.com>
Review of attachment 343453 [details] [review]: ::: gst/vaapi/gstvaapisink.c @@ +1461,2 @@ gst_vaapi_display_unlock (GST_VAAPI_PLUGIN_BASE_DISPLAY (sink)); + gst_buffer_replace (&old_buf, NULL); I think this changes the behavior. Previously, sink->video_buffer was set to NULL at the end of this function, but now it's being set to a new reference to buffer. Was that intentional? If not, I think sink->video_buffer could just be assigned NULL on line 1458.
(In reply to Matt Staples from comment #10) > Review of attachment 343453 [details] [review] [review]: > > ::: gst/vaapi/gstvaapisink.c > @@ +1461,2 @@ > gst_vaapi_display_unlock (GST_VAAPI_PLUGIN_BASE_DISPLAY (sink)); > + gst_buffer_replace (&old_buf, NULL); > > I think this changes the behavior. Previously, sink->video_buffer was set > to NULL at the end of this function, but now it's being set to a new > reference to buffer. Was that intentional? > If not, I think sink->video_buffer could just be assigned NULL on line 1458. sink->video_buffer wasn't never set to NULL inside gst_vaapisink_show_frame_unlocked(), since buffer always has a value, it always has have a reference to this buffer. (In reply to Matt Staples from comment #7) > I like the general approach. It certainly simplifies the overlay_expose > method. > However, the race condition is still there as long as the call, > "gst_buffer_replace(&sink->videoBuffer, buffer);" is made outside the lock. Giving a second thought, the race condition is not there anymore, IIUC, since sink->videoBuffer now is not read/write outside of gst_vaapisink_show_frame_unlocked(), so we can call gst_buffer_replace (sink->video_buffer, buffer) with the display unlocked. Nonetheless, I think we'll do it as you propose since it is clearer, IMO. Do you agree?
Sorry, my mistake - I see now that sink->video_buffer had previously been replaced with buffer, not NULL. So please ignore my previous comment. I think your last attachment looks correct as-is.
Attachment 343453 [details] pushed as f536028 - vaapisink: don't use member variable outside lock
Also pushed in branch 1.10 commit a7d536301be72f6f83663740169d885048f4c175 Author: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com> Date: Thu Jan 12 19:54:41 2017 +0100 vaapisink: don't use member variable outside lock Thus a race condition segfault is avoided. Original-patch-by: Matt Staples <staples255@gmail.com> https://bugzilla.gnome.org/show_bug.cgi?id=777146