From 7e65871ccc49f6f25f8782789283cf2afef6e86b Mon Sep 17 00:00:00 2001 From: Karsten Loesing Date: Mon, 25 Jan 2010 18:44:17 +0000 Subject: [PATCH] Fix a memory corruption bug while collecting bridge stats We accidentally freed the internal buffer for bridge stats when we were writing the bridge stats file or honoring a control port request for said data. Change the interfaces for geoip_get_bridge_stats* to prevent these problems, and remove the offending free/add a tor_strdup. Fixes bug 1208. --- ChangeLog | 7 +++++++ src/or/control.c | 4 ++-- src/or/geoip.c | 4 ++-- src/or/microdesc.c | 3 ++- src/or/or.h | 4 ++-- src/or/router.c | 18 +++++++++++------- 6 files changed, 26 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index c65f63c4f1..9beb307213 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,11 @@ Changes in version 0.2.2.8-alpha - 2010-??-?? + o Major bugfixes: + - Fix a memory corruption bug on bridges that occured during the + inclusion of stats data in extra-info descriptors. Also fix the + interface for geoip_get_bridge_stats* to prevent similar bugs in + the future. Diagnosis by Tas, patch by Karsten and Sebastian. + Fixes bug 1208; bugfix on 0.2.2.7-alpha. + o Minor bugfixes: - Ignore OutboundBindAddress when connecting to localhost. Connections to localhost need to come _from_ localhost, or else diff --git a/src/or/control.c b/src/or/control.c index be1e921f31..13a5f46b11 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -1755,10 +1755,10 @@ getinfo_helper_events(control_connection_t *control_conn, "information", question); } } else if (!strcmp(question, "status/clients-seen")) { - char *bridge_stats = geoip_get_bridge_stats_controller(time(NULL)); + const char *bridge_stats = geoip_get_bridge_stats_controller(time(NULL)); if (!bridge_stats) return -1; - *answer = bridge_stats; + *answer = tor_strdup(bridge_stats); } else { return 0; } diff --git a/src/or/geoip.c b/src/or/geoip.c index a00280e39d..7208b89865 100644 --- a/src/or/geoip.c +++ b/src/or/geoip.c @@ -1254,7 +1254,7 @@ load_bridge_stats(time_t now) /** Return most recent bridge statistics for inclusion in extra-info * descriptors, or NULL if we don't have recent bridge statistics. */ -char * +const char * geoip_get_bridge_stats_extrainfo(time_t now) { load_bridge_stats(now); @@ -1263,7 +1263,7 @@ geoip_get_bridge_stats_extrainfo(time_t now) /** Return most recent bridge statistics to be returned to controller * clients, or NULL if we don't have recent bridge statistics. */ -char * +const char * geoip_get_bridge_stats_controller(time_t now) { load_bridge_stats(now); diff --git a/src/or/microdesc.c b/src/or/microdesc.c index a754200764..f29611f930 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -77,7 +77,8 @@ dump_microdescriptor(FILE *f, microdesc_t *md, size_t *annotation_len_out) md->off = (off_t) ftell(f); written = fwrite(md->body, 1, md->bodylen, f); if (written != md->bodylen) { - log_warn(LD_DIR, "Couldn't dump microdescriptor (wrote %lu out of %lu): %s", + log_warn(LD_DIR, + "Couldn't dump microdescriptor (wrote %lu out of %lu): %s", (unsigned long)written, (unsigned long)md->bodylen, strerror(ferror(f))); return -1; diff --git a/src/or/or.h b/src/or/or.h index 091d819f79..e0b86387fa 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4137,8 +4137,8 @@ void geoip_entry_stats_init(time_t now); void geoip_entry_stats_write(time_t now); void geoip_bridge_stats_init(time_t now); time_t geoip_bridge_stats_write(time_t now); -char *geoip_get_bridge_stats_extrainfo(time_t); -char *geoip_get_bridge_stats_controller(time_t); +const char *geoip_get_bridge_stats_extrainfo(time_t); +const char *geoip_get_bridge_stats_controller(time_t); /********************************* hibernate.c **********************/ diff --git a/src/or/router.c b/src/or/router.c index 827df0302c..257bca935b 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1898,6 +1898,10 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, extrainfo->nickname, identity, published, bandwidth_usage); + tor_free(bandwidth_usage); + if (result<0) + return -1; + if (options->ExtraInfoStatistics && write_stats_to_extrainfo) { char *contents = NULL; log_info(LD_GENERAL, "Adding stats to extra-info descriptor."); @@ -1910,6 +1914,7 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, log_warn(LD_DIR, "Could not write dirreq-stats to extra-info " "descriptor."); s[pos] = '\0'; + write_stats_to_extrainfo = 0; } tor_free(contents); } @@ -1922,6 +1927,7 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, log_warn(LD_DIR, "Could not write entry-stats to extra-info " "descriptor."); s[pos] = '\0'; + write_stats_to_extrainfo = 0; } tor_free(contents); } @@ -1934,6 +1940,7 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, log_warn(LD_DIR, "Could not write buffer-stats to extra-info " "descriptor."); s[pos] = '\0'; + write_stats_to_extrainfo = 0; } tor_free(contents); } @@ -1946,17 +1953,14 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, log_warn(LD_DIR, "Could not write exit-stats to extra-info " "descriptor."); s[pos] = '\0'; + write_stats_to_extrainfo = 0; } tor_free(contents); } } - tor_free(bandwidth_usage); - if (result<0) - return -1; - - if (should_record_bridge_info(options)) { - char *bridge_stats = geoip_get_bridge_stats_extrainfo(now); + if (should_record_bridge_info(options) && write_stats_to_extrainfo) { + const char *bridge_stats = geoip_get_bridge_stats_extrainfo(now); if (bridge_stats) { size_t pos = strlen(s); if (strlcpy(s + pos, bridge_stats, maxlen - strlen(s)) != @@ -1964,8 +1968,8 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo, log_warn(LD_DIR, "Could not write bridge-stats to extra-info " "descriptor."); s[pos] = '\0'; + write_stats_to_extrainfo = 0; } - tor_free(bridge_stats); } } -- 2.11.4.GIT