[Bug 797203] shmsrc: race condition when stopping

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=797203

Olivier Crête <olivier.crete at ocrete.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #373760|none                        |needs-work
             status|                            |

--- Comment #2 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 373760:
 --> (https://bugzilla.gnome.org/review?bug=797203&attachment=373760)

::: sys/shm/gstshmsrc.c
@@ +278,2 @@
     gst_shm_pipe_dec (self->pipe);
+    GST_OBJECT_LOCK (self);

Are you sure this unlock+relock is not racy?

@@ +382,3 @@
   gsb = g_slice_new0 (struct GstShmBuffer);
   gsb->buf = buf;
   gsb->pipe = self->pipe;

There is a problem here, if the self->pipe can be NULL here, we'd get an
invalid GstShmBuffer.. My guess is that it shouldn't UNLOCK it at the end of
the while loop if buf != NULL... But in that case it's super ugly.

I think it should something like:

LOCK();
do {
  UNLOCK();
  poll_wait..()
  LOCK()

  if (test for error condition 1)
   goto error_condition_1;


} while ();

...
gsb->pipe = self->pipe;
UNLOCK;

return GST_FLOW_OK;

error_condition_1:
 UNLOCK();
 GST_ELEMENT_ERROR(...);
 return GST_FLOW_...;

@@ +431,2 @@
   self->unlocked = TRUE;
+  GST_OBJECT_UNLOCK (self);

should probably hold the lock across the set flushing for consistency.

@@ +445,2 @@
   self->unlocked = FALSE;
+  GST_OBJECT_UNLOCK (self);

should probably hold the lock across the set flushing for consistency here too.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.