Commit c2fe4e58 authored by Alex Ashley's avatar Alex Ashley Committed by Tim-Philipp Müller

curlhttpsrc: fix various leaks and thread safety issues

curlhttpsrc uses a single thread running the
gst_curl_http_src_curl_multi_loop() function to handle receiving
data and messages from libcurl. Each instance of curlhttpsrc adds
an entry into a queue in GstCurlHttpSrcMultiTaskContext and waits
for the multi_loop to perform the HTTP request.

Valgrind has shown up race conditions and memory leaks:
1. gst_curl_http_src_change_state() does not wait for the multi_loop
   to complete before going to the NULL state, which means that
   an instance of GstCurlHttpSrc can be released while
   gst_curl_http_src_curl_multi_loop() still has a reference to it.
2. if multiple elements try to be removed from the queue at once,
   only the last one is deleted.
3. source->caps is leaked
4. curl multi_handle is leaked
5. leak of curl_handle if URI not set
6. leak of http_headers when reusing element
7. null pointer dereference in negotiate caps
8. double-free of the default user-agent string
9. leak of multi_task_context.task

This commit changes the logic so that each element has a connection
status, which is used by the multi_loop to decide when to remove an
element from its queue. An instance of curlhttpsrc will not enter
the NULL state until its reference has been removed from the queue.

When shutting down the curl multi loop, the memory allocated from the
call to curl_multi_init() is now released.

When gstadaptivedemux uses a URI source element, it will re-use
it for multiple requests, moving it between READY and PLAYING
between each request. curlhttpsrc was leaking the http_headers
structure in this use case.

The gst_curl_http_src_negotiate_caps() function extracts the
"response-headers" field from the http_headers, but did not check
that this field might be NULL.

If the user-agent property is set, the global user-agent string
was freed. This caused a double-free error if the user-agent is
ever set a second time during the execution of the process.

There are situations within curlhttpsrc where the code needs
both the global multi_task_context mutex and the per-element
buffer_mutex. To avoid deadlocks, it is vital that the order in
which these are requested is always the same. This commit modifies
the locking order to always be in the order:
 1. multi_task_context.task_rec_mutex
 2. buffer_mutex

Fixes #876
parent d2d912f3
......@@ -60,7 +60,7 @@
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_PROXY ((void *)0)
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_PROXYUSERNAME ((void *)0)
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_PROXYPASSWORD ((void *)0)
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_USERAGENT gst_curl_http_src_default_useragent
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_USERAGENT "GStreamer curlhttpsrc libcurl"
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_ACCEPT_ENCODING FALSE
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_FOLLOWLOCATION 1L
#define GSTCURL_HANDLE_DEFAULT_CURLOPT_MAXREDIRS -1
......
This diff is collapsed.
......@@ -110,18 +110,12 @@ struct _GstCurlHttpSrcMultiTaskContext
guint refcount;
GCond signal;
GstCurlHttpSrc *request_removal_element;
GstCurlHttpSrcQueueElement *queue;
enum
{
GSTCURL_MULTI_LOOP_STATE_WAIT = 0,
GSTCURL_MULTI_LOOP_STATE_QUEUE_EVENT,
GSTCURL_MULTI_LOOP_STATE_RUNNING,
GSTCURL_MULTI_LOOP_STATE_REQUEST_REMOVAL,
GSTCURL_MULTI_LOOP_STATE_STOP,
GSTCURL_MULTI_LOOP_STATE_MAX
GSTCURL_MULTI_LOOP_STATE_STOP
} state;
/* < private > */
......@@ -200,11 +194,16 @@ struct _GstCurlHttpSrc
} state, pending_state;
CURL *curl_handle;
GMutex buffer_mutex;
GCond signal;
GCond buffer_cond;
gchar *buffer;
guint buffer_len;
gboolean transfer_begun;
gboolean data_received;
enum {
GSTCURL_NOT_CONNECTED,
GSTCURL_CONNECTED,
GSTCURL_WANT_REMOVAL
} connection_status;
/*
* Response Headers
......@@ -220,34 +219,6 @@ struct _GstCurlHttpSrc
GstCaps *caps;
};
enum
{
PROP_0,
PROP_URI,
PROP_USERNAME,
PROP_PASSWORD,
PROP_PROXYURI,
PROP_PROXYUSERNAME,
PROP_PROXYPASSWORD,
PROP_COOKIES,
PROP_USERAGENT,
PROP_HEADERS,
PROP_COMPRESS,
PROP_REDIRECT,
PROP_MAXREDIRECT,
PROP_KEEPALIVE,
PROP_TIMEOUT,
PROP_STRICT_SSL,
PROP_SSL_CA_FILE,
PROP_RETRIES,
PROP_CONNECTIONMAXTIME,
PROP_MAXCONCURRENT_SERVER,
PROP_MAXCONCURRENT_PROXY,
PROP_MAXCONCURRENT_GLOBAL,
PROP_HTTPVERSION,
PROP_MAX
};
GType gst_curl_http_src_get_type (void);
G_END_DECLS
......
......@@ -83,8 +83,9 @@ gst_curl_http_src_add_queue_item (GstCurlHttpSrcQueueElement ** queue,
}
insert_point->p = s;
g_mutex_init (&insert_point->running);
g_atomic_int_set (&insert_point->running, 0);
insert_point->next = NULL;
s->connection_status = GSTCURL_CONNECTED;
return TRUE;
}
......@@ -127,6 +128,7 @@ gst_curl_http_src_remove_queue_item (GstCurlHttpSrcQueueElement ** queue,
prev_qelement->next = this_qelement->next;
}
g_free (this_qelement);
s->connection_status = GSTCURL_NOT_CONNECTED;
return TRUE;
}
......@@ -164,12 +166,13 @@ gst_curl_http_src_remove_queue_handle (GstCurlHttpSrcQueueElement ** queue,
this_qelement->p->uri); */
/* First, signal the transfer owner thread to wake up */
g_mutex_lock (&this_qelement->p->buffer_mutex);
g_cond_signal (&this_qelement->p->signal);
g_cond_signal (&this_qelement->p->buffer_cond);
if (this_qelement->p->state != GSTCURL_UNLOCK) {
this_qelement->p->state = GSTCURL_DONE;
} else {
this_qelement->p->pending_state = GSTCURL_DONE;
}
this_qelement->p->connection_status = GSTCURL_NOT_CONNECTED;
this_qelement->p->curl_result = result;
g_mutex_unlock (&this_qelement->p->buffer_mutex);
......
......@@ -51,7 +51,7 @@
struct _GstCurlHttpSrcQueueElement
{
GstCurlHttpSrc *p;
GMutex running;
volatile gint running;
GstCurlHttpSrcQueueElement *next;
};
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment