From c7e421712155cc01b42bee9adea253134ceea5c1 Mon Sep 17 00:00:00 2001 From: Sebastian Rasmussen Date: Thu, 9 Jun 2016 01:20:36 +0200 Subject: [PATCH] curlsmtpsink: Lock and don't send final boundary upon error Previously GstCurlSmtpSink could cause the pipeline thread to end up waiting for a stopped thread to perform work. The scenario was that the sink could be rendering a buffer and waiting for the curl transfer thread to have sent the data. As soon as the transfer thread has copied all data to curl's data buffer in gst_curl_base_sink_transfer_read_cb() then the render call would stop waiting and return GST_FLOW_OK. While this takes place the transfer thread may suffer from an error e.g. due gst_poll_wait() timing out. This causes the transfer thread to record the error, claim (it is not really true since there was an error) that the data has been sent and that a response has been received by trying to signal the pipeline thread (but this has already stopped waiting). Finally the transfer thread stops itself. A short while later the pipeline thread may attempt to push an EOS event into GstCurlSmtpSink. Since there is no check in gst_curl_smtp_sink_event() to check if the sink has suffered from any error it may attempt to add a final boundary and ask the, now deceased, transfer thread to transfer the new data. Next the sink element would have waited for the transfer to complete (using a different mechanism than normal transfers through GstCurlBaseSink). In this case there was an error check to avoid waiting if an error had already been seen. Finally GstCurlSmtpSink would chain up to GstCurlBaseSink which would then block waiting for a response (normally this would be prevented by the transfer thread suffering the error claiming that it had been received, but GstCurlSmtpSink clobbered this flag after the fact). Now GstCurlSmtpSink avoids this by locking over the entire event handing (preventing simultaneous changes to flags by the two threads) and also by avoiding to initiate transfer of final boundary if an error has already been seen. Also add GST_FIXME() for remaining similar issue where the pipeline thread may block indefinitely waiting for transfer thread to transfer data but the transfer thread errors out and fails to notify the pipeline thread that the transfer failed. https://bugzilla.gnome.org/show_bug.cgi?id=767501 --- ext/curl/gstcurlsmtpsink.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/ext/curl/gstcurlsmtpsink.c b/ext/curl/gstcurlsmtpsink.c index 40ab016057..07a147430a 100644 --- a/ext/curl/gstcurlsmtpsink.c +++ b/ext/curl/gstcurlsmtpsink.c @@ -202,21 +202,16 @@ gst_curl_smtp_sink_event (GstBaseSink * bsink, GstEvent * event) GST_OBJECT_LOCK (sink); sink->eos = TRUE; - GST_OBJECT_UNLOCK (sink); - - if (sink->base64_chunk != NULL && !sink->final_boundary_added) { + if (bcsink->flow_ret == GST_FLOW_OK && sink->base64_chunk != NULL + && !sink->final_boundary_added) { add_final_boundary_unlocked (sink); - gst_curl_base_sink_transfer_thread_notify_unlocked (bcsink); - - GST_OBJECT_LOCK (sink); - if (sink->base64_chunk != NULL && bcsink->flow_ret == GST_FLOW_OK) { - gst_curl_smtp_sink_wait_for_transfer_end_unlocked (sink); - } - GST_OBJECT_UNLOCK (sink); + GST_FIXME_OBJECT (sink, "if gstpoll errors in transfer thread, then " + "this wait will never timeout because the transfer thread does " + "not signal it upon errors"); + gst_curl_smtp_sink_wait_for_transfer_end_unlocked (sink); } - gst_curl_base_sink_transfer_thread_close (bcsink); - + GST_OBJECT_UNLOCK (sink); break; default: