second version of this patch (fixed double free bug).
Alexander Malysh wrote:
> Hi,
>
> attached you can find path that fixes a problem with dead connections in
> http client connection pool. It's only a first part of fixes that we need
> (we still need to kill inactive connection after timeout expired).
>
> How it works is simple:
> 1) before putting connection into connection pool register dummy callback
> 2) if dummy callback called (we will receive error poll event) check if
> connection still ok and if not destroy it
> 3) before return connection from pool, unregister dummy callback
>
> Comments/votes please!
>
--
Thanks,
Alex
Index: gwlib/http.c
===================================================================
RCS file: /home/cvs/gateway/gwlib/http.c,v
retrieving revision 1.219
diff -a -u -p -r1.219 http.c
--- gwlib/http.c 11 Aug 2004 16:41:29 -0000 1.219
+++ gwlib/http.c 2 Nov 2004 00:21:58 -0000
@@ -601,6 +601,25 @@ static int entity_read(HTTPEntity *ent,
* HTTP client interface.
*/
+/*
+ * Internal lists of completely unhandled requests and requests for which
+ * a request has been sent but response has not yet been read.
+ */
+static List *pending_requests = NULL;
+
+
+/*
+ * Have background threads been started?
+ */
+static Mutex *client_thread_lock = NULL;
+static volatile sig_atomic_t client_threads_are_running = 0;
+
+
+/*
+ * Set of all connections to all servers. Used with conn_register to
+ * do I/O on several connections with a single thread.
+ */
+static FDSet *client_fdset = NULL;
/*
* Maximum number of HTTP redirections to follow. Making this infinite
@@ -629,7 +648,7 @@ typedef struct {
List *request_headers;
Octstr *request_body; /* NULL for GET or HEAD, non-NULL for POST */
enum {
- connecting,
+ connecting,
request_not_sent,
reading_status,
reading_entity,
@@ -707,17 +726,13 @@ static void server_destroy(void *p)
* Pool of open, but unused connections to servers or proxies. Key is
* "servername:port", value is List with Connection objects.
*/
-static Dict *conn_pool = NULL;
-static Mutex *conn_pool_lock = NULL;
+static Dict *conn_pool;
+static Mutex *conn_pool_lock;
static void conn_pool_item_destroy(void *item)
{
- Connection *conn;
-
- while ((conn = list_extract_first(item)) != NULL)
- conn_destroy(conn);
- list_destroy(item, NULL);
+ list_destroy(item, (void(*)(void*))conn_destroy);
}
static void conn_pool_init(void)
@@ -734,7 +749,7 @@ static void conn_pool_shutdown(void)
}
-static Octstr *conn_pool_key(Octstr *host, int port)
+static inline Octstr *conn_pool_key(Octstr *host, int port)
{
return octstr_format("%S:%d", host, port);
}
@@ -754,15 +769,19 @@ static Connection *conn_pool_get(Octstr
if (list == NULL)
conn = NULL;
else {
- while (1) {
- conn = list_extract_first(list);
- if (conn == NULL)
- break;
- /* Check whether the server has closed the connection while
- * it has been in the pool. */
- conn_wait(conn, 0);
- if (!conn_eof(conn) && !conn_error(conn))
- break;
+ while ((conn = list_extract_first(list)) != NULL) {
+ /*
+ * Check whether the server has closed the connection while
+ * it has been in the pool.
+ */
+ conn_wait(conn, 0);
+ if (!conn_eof(conn) && !conn_error(conn)) {
+#ifdef USE_KEEPALIVE
+ /* unregister our server disconnect callback */
+ conn_unregister(conn);
+#endif
+ break;
+ }
conn_destroy(conn);
}
}
@@ -786,6 +805,42 @@ static Connection *conn_pool_get(Octstr
}
#ifdef USE_KEEPALIVE
+static void check_pool_conn(Connection *conn, void *data)
+{
+ Octstr *key = data;
+ if (run_status != running) {
+ octstr_destroy(key);
+ conn_unregister(conn);
+ return;
+ }
+ /* check if connection still ok */
+ conn_wait(conn, 0);
+ if (conn_error(conn) || conn_eof(conn)) {
+ List *list;
+ mutex_lock(conn_pool_lock);
+ list = dict_get(conn_pool, key);
+ if (list_delete_equal(list, conn) > 0) {
+ /*
+ * ok, connection was still within pool. So it's
+ * safe to destroy this connection.
+ */
+ debug("gwlib.http", 0, "HTTP: Server closed connection, destroying it <%s><%p>.",
+ octstr_get_cstr(key), conn);
+ conn_destroy(conn);
+ octstr_destroy(key);
+ } else {
+ /* hmm, bad... connections is already used */
+ error(0, "HTTP: Race condition detected: Could not found connection in pool!");
+ /*
+ * don't unregister or destroy our key.
+ * Better memleak as segfault ;)
+ */
+ }
+ mutex_unlock(conn_pool_lock);
+ }
+}
+
+
static void conn_pool_put(Connection *conn, Octstr *host, int port)
{
Octstr *key;
@@ -799,33 +854,13 @@ static void conn_pool_put(Connection *co
dict_put(conn_pool, key, list);
}
list_append(list, conn);
- octstr_destroy(key);
+ /* register connection to get server disconnect */
+ conn_register(conn, client_fdset, check_pool_conn, key);
mutex_unlock(conn_pool_lock);
}
#endif
-/*
- * Internal lists of completely unhandled requests and requests for which
- * a request has been sent but response has not yet been read.
- */
-static List *pending_requests = NULL;
-
-
-/*
- * Have background threads been started?
- */
-static Mutex *client_thread_lock = NULL;
-static volatile sig_atomic_t client_threads_are_running = 0;
-
-
-/*
- * Set of all connections to all servers. Used with conn_register to
- * do I/O on several connections with a single thread.
- */
-static FDSet *client_fdset = NULL;
-
-
HTTPCaller *http_caller_create(void)
{
HTTPCaller *caller;
@@ -1025,7 +1060,7 @@ static void handle_transaction(Connectio
#ifdef DUMP_RESPONSE
/* Dump the response */
- debug("wsp.http", 0, "HTTP: Received response:");
+ debug("gwlib.http", 0, "HTTP: Received response:");
h = build_response(trans->response->headers, trans->response->body);
octstr_dump(h, 0);
octstr_destroy(h);
@@ -1051,11 +1086,11 @@ static void handle_transaction(Connectio
if (trans->persistent) {
if (proxy_used_for_host(trans->host))
conn_pool_put(trans->conn, proxy_hostname, proxy_port);
- else
+ else
conn_pool_put(trans->conn, trans->host, trans->port);
} else
#endif
- conn_destroy(trans->conn);
+ conn_destroy(trans->conn);
trans->conn = NULL;
@@ -1422,13 +1457,11 @@ static void parse2trans(HTTPURLParse *p,
static Connection *get_connection(HTTPServer *trans)
{
- Connection *conn;
+ Connection *conn = NULL;
Octstr *host;
HTTPURLParse *p;
int port;
- conn = NULL;
-
/* if the parsing has not yet been done, then do it now */
if (!trans->host && trans->port == 0 && trans->url != NULL) {
if ((p = parse_url(trans->url)) != NULL) {
@@ -1447,26 +1480,17 @@ static Connection *get_connection(HTTPSe
port = trans->port;
}
- if (trans->retrying) {
-#ifdef HAVE_LIBSSL
- if (trans->ssl) conn = conn_open_ssl(host, port, trans->certkeyfile, http_interface);
- else
-#endif /* HAVE_LIBSSL */
- conn = conn_open_tcp_nb(host, port, http_interface);
- debug("gwlib.http", 0, "HTTP: Opening NEW connection to `%s:%d' (fd=%d).",
- octstr_get_cstr(host), port, conn_get_id(conn));
- } else
conn = conn_pool_get(host, port, trans->ssl, trans->certkeyfile,
http_interface);
if (conn == NULL)
goto error;
- return conn;
+ return conn;
- error:
- conn_destroy(conn);
- error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
- return NULL;
+error:
+ conn_destroy(conn);
+ error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
+ return NULL;
}
@@ -1476,47 +1500,47 @@ static Connection *get_connection(HTTPSe
*/
static int send_request(HTTPServer *trans)
{
- Octstr *request;
+ Octstr *request;
+
+ request = NULL;
- request = NULL;
+ /*
+ * we have to assume all values in trans are already set
+ * by parse_url() before calling this.
+ */
+
+ if (trans->username != NULL)
+ http_add_basic_auth(trans->request_headers, trans->username,
+ trans->password);
- /*
- * we have to assume all values in trans are already set
- * by parse_url() before calling this.
- */
-
- if (trans->username != NULL)
- http_add_basic_auth(trans->request_headers, trans->username,
- trans->password);
-
- if (proxy_used_for_host(trans->host)) {
- proxy_add_authentication(trans->request_headers);
- request = build_request(http_method2name(trans->method),
- trans->url, trans->host, trans->port,
- trans->request_headers,
- trans->request_body);
- } else {
- request = build_request(http_method2name(trans->method), trans->uri,
- trans->host, trans->port,
- trans->request_headers,
- trans->request_body);
- }
+ if (proxy_used_for_host(trans->host)) {
+ proxy_add_authentication(trans->request_headers);
+ request = build_request(http_method2name(trans->method),
+ trans->url, trans->host, trans->port,
+ trans->request_headers,
+ trans->request_body);
+ } else {
+ request = build_request(http_method2name(trans->method), trans->uri,
+ trans->host, trans->port,
+ trans->request_headers,
+ trans->request_body);
+ }
- debug("wsp.http", 0, "HTTP: Sending request:");
+ debug("gwlib.http", 0, "HTTP: Sending request:");
octstr_dump(request, 0);
- if (conn_write(trans->conn, request) == -1)
+ if (conn_write(trans->conn, request) == -1)
goto error;
octstr_destroy(request);
- return 0;
+ return 0;
- error:
- conn_destroy(trans->conn);
- trans->conn = NULL;
+error:
+ conn_destroy(trans->conn);
+ trans->conn = NULL;
octstr_destroy(request);
error(0, "Couldn't send request to <%s>", octstr_get_cstr(trans->url));
- return -1;
+ return -1;
}
@@ -1544,46 +1568,43 @@ static void write_request_thread(void *a
*/
trans->conn = get_connection(trans);
- if (trans->conn == NULL)
- list_produce(trans->caller, trans);
- else {
- if (conn_is_connected(trans->conn) == 0) {
- debug("gwlib.http", 0, "Socket connected at once");
-
- if (trans->method == HTTP_METHOD_POST) {
- /*
- * Add a Content-Length header. Override an existing one, if
- * necessary. We must have an accurate one in order to use the
- * connection for more than a single request.
- */
- http_header_remove_all(trans->request_headers, "Content-Length");
- sprintf(buf, "%ld", octstr_len(trans->request_body));
- http_header_add(trans->request_headers, "Content-Length", buf);
- }
+ if (trans->conn == NULL)
+ list_produce(trans->caller, trans);
+ else if (conn_is_connected(trans->conn) == 0) {
+ debug("gwlib.http", 0, "Socket connected at once");
+
+ if (trans->method == HTTP_METHOD_POST) {
+ /*
+ * Add a Content-Length header. Override an existing one, if
+ * necessary. We must have an accurate one in order to use the
+ * connection for more than a single request.
+ */
+ http_header_remove_all(trans->request_headers, "Content-Length");
+ sprintf(buf, "%ld", octstr_len(trans->request_body));
+ http_header_add(trans->request_headers, "Content-Length", buf);
+ }
/*
* ok, this has to be an GET or HEAD request method then,
* if it contains a body, then this is not HTTP conform, so at
* least warn the user
*/
- else if (trans->request_body != NULL) {
- warning(0, "HTTP: GET or HEAD method request contains body:");
- octstr_dump(trans->request_body, 0);
- }
- if ((rc = send_request(trans)) == 0) {
- trans->state = reading_status;
- conn_register(trans->conn, client_fdset, handle_transaction,
- trans);
+ else if (trans->request_body != NULL) {
+ warning(0, "HTTP: GET or HEAD method request contains body:");
+ octstr_dump(trans->request_body, 0);
+ }
+ if ((rc = send_request(trans)) == 0) {
+ trans->state = reading_status;
+ conn_register(trans->conn, client_fdset, handle_transaction,
+ trans);
} else {
- list_produce(trans->caller, trans);
+ list_produce(trans->caller, trans);
}
- } else { /* Socket not connected, wait for connection */
+ } else { /* Socket not connected, wait for connection */
debug("gwlib.http", 0, "Socket connecting");
trans->state = connecting;
conn_register(trans->conn, client_fdset, handle_transaction, trans);
}
-
- }
}
}
@@ -1609,7 +1630,7 @@ static void start_client_threads(void)
void http_set_interface(const Octstr *our_host)
{
- http_interface = octstr_duplicate(our_host);
+ http_interface = octstr_duplicate(our_host);
}
@@ -2656,7 +2677,7 @@ Octstr *http_header_value(List *headers,
List *http_header_duplicate(List *headers)
{
List *new;
- long i;
+ long i, len;
gwlib_assert_init();
@@ -2664,13 +2685,14 @@ List *http_header_duplicate(List *header
return NULL;
new = http_create_empty_headers();
- for (i = 0; i < list_len(headers); ++i)
+ len = list_len(headers);
+ for (i = 0; i < len; ++i)
list_append(new, octstr_duplicate(list_get(headers, i)));
return new;
}
-#define MAX_HEADER_LENGHT 256
+#define MAX_HEADER_LENGTH 256
/*
* Aggregate header in one (or more) lines with several parameters separated
* by commas, instead of one header per parameter
@@ -2697,7 +2719,7 @@ void http_header_pack(List *headers)
http_header_get(headers, j, &name2, &value2);
if(octstr_case_compare(name, name2) == 0) {
- if(octstr_len(value) + 2 + octstr_len(value2) > MAX_HEADER_LENGHT) {
+ if(octstr_len(value) + 2 + octstr_len(value2) > MAX_HEADER_LENGTH) {
octstr_destroy(name2);
octstr_destroy(value2);
break;
@@ -3278,9 +3300,9 @@ void http_shutdown(void)
run_status = terminating;
- conn_pool_shutdown();
port_shutdown();
client_shutdown();
+ conn_pool_shutdown();
server_shutdown();
proxy_shutdown();
#ifdef HAVE_LIBSSL
