From 4144b4552b00c25eba9be7a959aa75c1efbe4a18 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Aug 2014 11:33:05 -0400 Subject: Always event_del() connection events before freeing them Previously, we had done this only in the connection_free() case, but when we called connection_free_() directly from connections_free_all(), we didn't free the connections. --- changes/bug12985 | 4 ++++ src/common/compat_libevent.c | 14 +++++++++++++- src/common/compat_libevent.h | 5 ++--- src/or/connection.c | 6 ++++-- 4 files changed, 23 insertions(+), 6 deletions(-) create mode 100644 changes/bug12985 diff --git a/changes/bug12985 b/changes/bug12985 new file mode 100644 index 0000000000..dc14cdd375 --- /dev/null +++ b/changes/bug12985 @@ -0,0 +1,4 @@ + o Minor bugfixes (shutdown): + - When shutting down, always call event_del() on lingering read or + write events before freeing them. Fixes bug 12985; bugfix on + 0.1.0.2-rc. diff --git a/src/common/compat_libevent.c b/src/common/compat_libevent.c index 200a7c65fb..2a7386df4a 100644 --- a/src/common/compat_libevent.c +++ b/src/common/compat_libevent.c @@ -144,13 +144,25 @@ tor_evsignal_new(struct event_base * base, int sig, { return tor_event_new(base, sig, EV_SIGNAL|EV_PERSIST, cb, arg); } -/** Work-alike replacement for event_free() on pre-Libevent-2.0 systems. */ +/** Work-alike replacement for event_free() on pre-Libevent-2.0 systems, + * except tolerate tor_event_free(NULL). */ void tor_event_free(struct event *ev) { + if (ev == NULL) + return; event_del(ev); tor_free(ev); } +#else +/* Wrapper for event_free() that tolerates tor_event_free(NULL) */ +void +tor_event_free(struct event *ev) +{ + if (ev == NULL) + return; + event_free(ev); +} #endif /** Global event base for use by the main thread. */ diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index 2472e2c49e..ab7066beb2 100644 --- a/src/common/compat_libevent.h +++ b/src/common/compat_libevent.h @@ -28,11 +28,9 @@ void suppress_libevent_log_msg(const char *msg); #define tor_event_new event_new #define tor_evtimer_new evtimer_new #define tor_evsignal_new evsignal_new -#define tor_event_free event_free #define tor_evdns_add_server_port(sock, tcp, cb, data) \ evdns_add_server_port_with_base(tor_libevent_get_base(), \ (sock),(tcp),(cb),(data)); - #else struct event *tor_event_new(struct event_base * base, evutil_socket_t sock, short what, void (*cb)(evutil_socket_t, short, void *), void *arg); @@ -40,10 +38,11 @@ struct event *tor_evtimer_new(struct event_base * base, void (*cb)(evutil_socket_t, short, void *), void *arg); struct event *tor_evsignal_new(struct event_base * base, int sig, void (*cb)(evutil_socket_t, short, void *), void *arg); -void tor_event_free(struct event *ev); #define tor_evdns_add_server_port evdns_add_server_port #endif +void tor_event_free(struct event *ev); + typedef struct periodic_timer_t periodic_timer_t; periodic_timer_t *periodic_timer_new(struct event_base *base, diff --git a/src/or/connection.c b/src/or/connection.c index 4f74a1d04b..6d205d143c 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -553,8 +553,10 @@ connection_free_(connection_t *conn) tor_free(control_conn->incoming_cmd); } - tor_free(conn->read_event); /* Probably already freed by connection_free. */ - tor_free(conn->write_event); /* Probably already freed by connection_free. */ + /* Probably already freed by connection_free. */ + tor_event_free(conn->read_event); + tor_event_free(conn->write_event); + conn->read_event = conn->write_event = NULL; IF_HAS_BUFFEREVENT(conn, { /* This was a workaround to handle bugs in some old versions of libevent * where callbacks can occur after calling bufferevent_free(). Setting -- cgit v1.2.3-54-g00ecf From b82e166bec5fcc468424af1ff71e2e753ac534a2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Aug 2014 12:21:57 -0400 Subject: restore the sensible part of ac268a83408e1450544db2f23f364dfa3 We don't want to call event_del() postfork, if cpuworkers are multiprocess. --- src/or/connection.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/or/connection.c b/src/or/connection.c index 6d205d143c..aedb29d4e4 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -553,10 +553,17 @@ connection_free_(connection_t *conn) tor_free(control_conn->incoming_cmd); } +#ifdef TOR_IS_MULTITHREADED /* Probably already freed by connection_free. */ + /* We don't do these frees on the multiprocess case, since in that case we + * don't want to call event_del() postfork or it's likely to mess up. + * Multiprocess builds are deprecated, so let's just have a one-time memory + * leak here. + */ tor_event_free(conn->read_event); tor_event_free(conn->write_event); conn->read_event = conn->write_event = NULL; +#endif IF_HAS_BUFFEREVENT(conn, { /* This was a workaround to handle bugs in some old versions of libevent * where callbacks can occur after calling bufferevent_free(). Setting -- cgit v1.2.3-54-g00ecf From d8fe499e08cf6c04fd6bf6230c916dcedf17dc80 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 29 Aug 2014 12:25:05 -0400 Subject: Revert "restore the sensible part of ac268a83408e1450544db2f23f364dfa3" This reverts commit b82e166bec5fcc468424af1ff71e2e753ac534a2. We don't need that part in 0.2.5, since 0.2.5 no longer supports non-multithreaded builds. --- src/or/connection.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index bcb17376c0..36200e57ea 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -575,17 +575,10 @@ connection_free_(connection_t *conn) tor_free(control_conn->incoming_cmd); } -#ifdef TOR_IS_MULTITHREADED /* Probably already freed by connection_free. */ - /* We don't do these frees on the multiprocess case, since in that case we - * don't want to call event_del() postfork or it's likely to mess up. - * Multiprocess builds are deprecated, so let's just have a one-time memory - * leak here. - */ tor_event_free(conn->read_event); tor_event_free(conn->write_event); conn->read_event = conn->write_event = NULL; -#endif IF_HAS_BUFFEREVENT(conn, { /* This was a workaround to handle bugs in some old versions of libevent * where callbacks can occur after calling bufferevent_free(). Setting -- cgit v1.2.3-54-g00ecf From e9463b04e7c396d4344dc658e0c9dae3bdfc15af Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 7 Jan 2015 11:52:24 -0500 Subject: Clarify why bug12985 is a thing --- changes/bug12985 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/bug12985 b/changes/bug12985 index dc14cdd375..636ae4d564 100644 --- a/changes/bug12985 +++ b/changes/bug12985 @@ -1,4 +1,5 @@ o Minor bugfixes (shutdown): - When shutting down, always call event_del() on lingering read or - write events before freeing them. Fixes bug 12985; bugfix on + write events before freeing them. Otherwise, we risk double-frees + or read-after-frees in event_base_free(). Fixes bug 12985; bugfix on 0.1.0.2-rc. -- cgit v1.2.3-54-g00ecf