From 6d2e02d79bf3cbd5d35fd65068485520cd6ef84b Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Fri, 12 Nov 2010 00:21:03 -0800 Subject: [PATCH 1/7] Move the original log_info call out of the core of buf_shrink_freelists. Sending a log message to a control port can cause Tor to allocate a buffer, thereby changing the length of the freelist behind buf_shrink_freelists's back, thereby causing an assertion to fail. Fixes bug #1125. --- src/or/buffers.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 09ccb7cb0d..e835c615ef 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -269,16 +269,12 @@ buf_shrink_freelists(int free_all) int n_to_free = free_all ? freelists[i].cur_length : (freelists[i].lowest_length - slack); int n_to_skip = freelists[i].cur_length - n_to_free; + int orig_length = freelists[i].cur_length; int orig_n_to_free = n_to_free, n_freed=0; int orig_n_to_skip = n_to_skip; int new_length = n_to_skip; chunk_t **chp = &freelists[i].head; chunk_t *chunk; - log_info(LD_MM, "Cleaning freelist for %d-byte chunks: length %d, " - "keeping %d, dropping %d.", - (int)freelists[i].alloc_size, freelists[i].cur_length, - n_to_skip, n_to_free); - tor_assert(n_to_skip + n_to_free == freelists[i].cur_length); while (n_to_skip) { if (! (*chp)->next) { log_warn(LD_BUG, "I wanted to skip %d chunks in the freelist for " @@ -313,6 +309,10 @@ buf_shrink_freelists(int free_all) } // tor_assert(!n_to_free); freelists[i].cur_length = new_length; + log_info(LD_MM, "Cleaned freelist for %d-byte chunks: original " + "length %d, kept %d, dropped %d.", + (int)freelists[i].alloc_size, orig_length, + orig_n_to_skip, orig_n_to_free); } freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); From 6a0657d4bbb23858c9a01d5fbc2a2efdfee3a590 Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Fri, 12 Nov 2010 00:46:26 -0800 Subject: [PATCH 2/7] Disable logging to control port connections in buf_shrink_freelists. If buf_shrink_freelists calls log_warn for some reason, we don't want the log call itself to throw buf_shrink_freelists further off the rails. --- src/or/buffers.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/or/buffers.c b/src/or/buffers.c index e835c615ef..7c28dc1a47 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -262,6 +262,7 @@ buf_shrink_freelists(int free_all) { #ifdef ENABLE_BUF_FREELISTS int i; + disable_control_logging(); for (i = 0; freelists[i].alloc_size; ++i) { int slack = freelists[i].slack; assert_freelist_ok(&freelists[i]); @@ -317,6 +318,7 @@ buf_shrink_freelists(int free_all) freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); } + enable_control_logging(); #else (void) free_all; #endif From 81affe194905147f8e7692818dad786d3421f9a7 Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Fri, 12 Nov 2010 03:04:07 -0800 Subject: [PATCH 3/7] Move the original log_info call out of the core of buf_shrink_freelists. Sending a log message to a control port can cause Tor to allocate a buffer, thereby changing the length of the freelist behind buf_shrink_freelists's back, thereby causing an assertion to fail. Fixes bug #1125. --- src/or/buffers.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 571f86de1f..5b53d12f80 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -259,12 +259,10 @@ buf_shrink_freelists(int free_all) (freelists[i].lowest_length - slack); int n_to_skip = freelists[i].cur_length - n_to_free; int orig_n_to_free = n_to_free, n_freed=0; + int orig_n_to_skip = n_to_skip; int new_length = n_to_skip; chunk_t **chp = &freelists[i].head; chunk_t *chunk; - log_info(LD_MM, "Cleaning freelist for %d-byte chunks: keeping %d, " - "dropping %d.", - (int)freelists[i].alloc_size, n_to_skip, n_to_free); while (n_to_skip) { tor_assert((*chp)->next); chp = &(*chp)->next; @@ -291,6 +289,9 @@ buf_shrink_freelists(int free_all) } // tor_assert(!n_to_free); freelists[i].cur_length = new_length; + log_info(LD_MM, "Cleaned freelist for %d-byte chunks: kept %d, " + "dropped %d.", + (int)freelists[i].alloc_size, orig_n_to_skip, orig_n_to_free); } freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); From a421e284d068955783fa30d6b7088d605b440ffd Mon Sep 17 00:00:00 2001 From: Robert Ransom Date: Fri, 12 Nov 2010 03:07:09 -0800 Subject: [PATCH 4/7] Disable logging to control port connections in buf_shrink_freelists. If buf_shrink_freelists calls log_warn for some reason, we don't want the log call itself to throw buf_shrink_freelists further off the rails. --- src/or/buffers.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/or/buffers.c b/src/or/buffers.c index 5b53d12f80..b96b82de5a 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -251,6 +251,7 @@ buf_shrink_freelists(int free_all) { #ifdef ENABLE_BUF_FREELISTS int i; + disable_control_logging(); for (i = 0; freelists[i].alloc_size; ++i) { int slack = freelists[i].slack; assert_freelist_ok(&freelists[i]); @@ -296,6 +297,7 @@ buf_shrink_freelists(int free_all) freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); } + enable_control_logging(); #else (void) free_all; #endif From 566a115be1acbd0838c81edd251cf7ae47b94fe3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 12 Nov 2010 12:59:42 -0500 Subject: [PATCH 5/7] Add changes file for bug1125 --- changes/bug1125 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/bug1125 diff --git a/changes/bug1125 b/changes/bug1125 new file mode 100644 index 0000000000..1331246a14 --- /dev/null +++ b/changes/bug1125 @@ -0,0 +1,8 @@ + o Major bugfixes + - Do not log messages to the controller while shrinking buffer + freelists. Doing so would sometimes make the controller + connection try to allocate a buffer chunk, which would mess + up the internals of the freelist and cause an assertion + failure. Fixes bug 1125; fixed by Robert Ransom. Bugfix on + Tor 0.2.0.16-alpha. + From 3a7614c670cd31b76df1ddc8872e7050faf61355 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 12 Nov 2010 13:03:18 -0500 Subject: [PATCH 6/7] Add changes file for bug1125 --- changes/bug1125 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/bug1125 diff --git a/changes/bug1125 b/changes/bug1125 new file mode 100644 index 0000000000..1331246a14 --- /dev/null +++ b/changes/bug1125 @@ -0,0 +1,8 @@ + o Major bugfixes + - Do not log messages to the controller while shrinking buffer + freelists. Doing so would sometimes make the controller + connection try to allocate a buffer chunk, which would mess + up the internals of the freelist and cause an assertion + failure. Fixes bug 1125; fixed by Robert Ransom. Bugfix on + Tor 0.2.0.16-alpha. + From dbba84c917279c8c58b1bfdac37fbcdfd84b7bb7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 12 Nov 2010 13:05:58 -0500 Subject: [PATCH 7/7] Avoid perma-blocking the controller on bug in shrink_freelist In all likelihood, this bug would make Tor assert, but if it doesn't, let's not have two bugs. --- src/or/buffers.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 7c28dc1a47..d25ff64333 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -283,7 +283,7 @@ buf_shrink_freelists(int free_all) orig_n_to_skip, (int)freelists[i].alloc_size, orig_n_to_skip-n_to_skip, freelists[i].cur_length); assert_freelist_ok(&freelists[i]); - return; + goto done; } // tor_assert((*chp)->next); chp = &(*chp)->next; @@ -318,6 +318,7 @@ buf_shrink_freelists(int free_all) freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); } + done: enable_control_logging(); #else (void) free_all;