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.

diff --git a/ChangeLog b/ChangeLog
index c65f63c4f1a9b079830f6e0ba67d1bebe4534939..9beb307213b0de091e3f055199bdcecc280cd2d0 100644 (file)
--- 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 be1e921f31405f22894342dc8e4d5ae0ec45eb57..13a5f46b114994a3983b3a0b8bc952f1fd3939e3 100644 (file)
--- 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 a00280e39dafa079a77b80d12b126fbe1270e9d1..7208b89865b742657f4e2e96fc1c7d84d0ebc32f 100644 (file)
--- 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 a7542007640b4f0964698f201f4d2d33544b7558..f29611f930da236d465f329bfe6b328110e767f8 100644 (file)
--- 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 091d819f792704ac76c6015c93425d7364a274f0..e0b86387faff78b2ee3439f2a4d92d8c6c32269e 100644 (file)
--- 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 827df0302ce3faf32f0cd730e36b6424c2e1c8b4..257bca935bc5e9bc677dd2b668da4c639515053e 100644 (file)
--- 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);
     }
   }