From bda58a124af2293a462734acdc88e1a655966dfe Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sat, 4 Oct 2008 13:00:57 +0300 Subject: [PATCH 1/2] Revert "Use given connections id in connection_disappeared()." This reverts src/{network,sched}/connection.c CVS revision 1.43, which was made on 2003-07-03 and converted to Git commit cae65f7941628109b51ffb2e2d05882fbbdc73ef in elinks-history. It is pointless to check whether (c == d && c->id == d->id). If c == d, then surely c->id == d->id, and I wouldn't be surprised to see a compiler optimize that out. Whereas, by taking the id as a parameter, connection_disappeared() can check whether the pointer now points to a new struct connection with a different id. --- src/network/connection.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/network/connection.c b/src/network/connection.c index 2f4271bd0..61614ee80 100644 --- a/src/network/connection.c +++ b/src/network/connection.c @@ -118,13 +118,19 @@ get_connections_transfering_count(void) return i; } +/** Check whether the pointer @a conn still points to a connection + * with the given @a id. If the struct connection has already been + * freed, this returns 0. By comparing connection.id, this function + * can usually detect even the case where a different connection has + * been created at the same address. For that to work, the caller + * must save the connection.id before the connection can be deleted. */ static inline int -connection_disappeared(struct connection *conn) +connection_disappeared(struct connection *conn, unsigned int id) { struct connection *c; foreach (c, connection_queue) - if (conn == c && conn->id == c->id) + if (conn == c && id == c->id) return 0; return 1; @@ -349,9 +355,11 @@ set_connection_state(struct connection *conn, struct connection_state state) conn->state = state; if (is_in_state(conn->state, S_TRANS)) { if (progress->timer == TIMER_ID_UNDEF) { + const unsigned int id = conn->id; + start_update_progress(progress, (void (*)(void *)) stat_timer, conn); update_connection_progress(conn); - if (connection_disappeared(conn)) + if (connection_disappeared(conn, id)) return; } @@ -420,13 +428,15 @@ void notify_connection_callbacks(struct connection *conn) { struct connection_state state = conn->state; + unsigned int id = conn->id; struct download *download, *next; foreachsafe (download, next, conn->downloads) { download->cached = conn->cached; if (download->callback) download->callback(download, download->data); - if (is_in_progress_state(state) && connection_disappeared(conn)) + if (is_in_progress_state(state) + && connection_disappeared(conn, id)) return; } } From 00f583181220b3cac7e2585fbbd0aacb793f1ece Mon Sep 17 00:00:00 2001 From: Kalle Olavi Niemitalo Date: Sat, 4 Oct 2008 13:46:43 +0300 Subject: [PATCH 2/2] Bug 1053: Fix crash when download ends prematurely. Call stacks reported by valgrind: ==14702== at 0x80DD791: read_from_socket (socket.c:945) ==14702== by 0x8104D0C: read_more_http_data (http.c:1180) ==14702== by 0x81052FE: read_http_data (http.c:1388) ==14702== by 0x80DD69B: read_select (socket.c:910) ==14702== by 0x80D27AA: select_loop (select.c:307) ==14702== by 0x80D1ADE: main (main.c:358) ==14702== Address 0x4F4E598 is 56 bytes inside a block of size 81 free'd ==14702== at 0x402210F: free (vg_replace_malloc.c:233) ==14702== by 0x812BED8: debug_mem_free (memdebug.c:484) ==14702== by 0x80D7C82: done_connection (connection.c:479) ==14702== by 0x80D8A44: abort_connection (connection.c:769) ==14702== by 0x80D99CE: cancel_download (connection.c:1053) ==14702== by 0x8110EB6: abort_download (download.c:143) ==14702== by 0x81115BC: download_data_store (download.c:337) ==14702== by 0x8111AFB: download_data (download.c:446) ==14702== by 0x80D7B33: notify_connection_callbacks (connection.c:458) ==14702== by 0x80D781E: set_connection_state (connection.c:388) ==14702== by 0x80D7132: set_connection_socket_state (connection.c:234) ==14702== by 0x80DD78D: read_from_socket (socket.c:943) read_from_socket() attempted to read socket->fd in order to set handlers on it, but the socket had already been freed. Incidentally, socket->fd was -1, which would have resulted in an assertion failure if valgrind hadn't caught the bug first. To fix this, add a list of weak references to sockets. read_from_socket() registers a weak reference on entry and unregisters it before exit. done_socket() breaks any weak references to the specified socket. read_from_socket() then checks whether the weak reference was broken, and doesn't access the socket any more if so. --- src/network/socket.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/network/socket.c b/src/network/socket.c index 4179b1526..e0f6b4024 100644 --- a/src/network/socket.c +++ b/src/network/socket.c @@ -69,6 +69,16 @@ struct connect_info { struct uri *uri; /* For updating the blacklist. */ }; +/** For detecting whether a struct socket has been deleted while a + * function was using it. */ +struct socket_weak_ref { + LIST_HEAD(struct socket_weak_ref); + + /** done_socket() resets this to NULL. */ + struct socket *socket; +}; + +static INIT_LIST_OF(struct socket_weak_ref, socket_weak_refs); /* To enable logging of tranfers, for debugging purposes. */ #if 0 @@ -143,6 +153,8 @@ init_socket(void *conn, struct socket_operations *ops) void done_socket(struct socket *socket) { + struct socket_weak_ref *ref; + close_socket(socket); if (socket->connect_info) @@ -150,6 +162,11 @@ done_socket(struct socket *socket) mem_free_set(&socket->read_buffer, NULL); mem_free_set(&socket->write_buffer, NULL); + + foreach(ref, socket_weak_refs) { + if (ref->socket == socket) + ref->socket = NULL; + } } void @@ -935,13 +952,26 @@ void read_from_socket(struct socket *socket, struct read_buffer *buffer, struct connection_state state, socket_read_T done) { + const int is_buffer_new = (buffer != socket->read_buffer); + struct socket_weak_ref ref; select_handler_T write_handler; + ref.socket = socket; + add_to_list(socket_weak_refs, &ref); + buffer->done = done; socket->ops->set_timeout(socket, connection_state(0)); socket->ops->set_state(socket, state); + del_from_list(&ref); + if (ref.socket == NULL) { + /* socket->ops->set_state deleted the socket. */ + if (is_buffer_new) + mem_free(buffer); + return; + } + if (socket->read_buffer && buffer != socket->read_buffer) mem_free(socket->read_buffer); socket->read_buffer = buffer;