Add some fixes after discussion with Nick.

- Refactor geoip.c by moving duplicate code into rotate_request_period().
- Don't leak memory when cleaning up cell queues.
- Make sure that exit_(streams|bytes_(read|written)) are initialized in all
  places accessing these arrays.
- Read only the last block from *stats files and ensure that its timestamp
  is not more than 25 hours in the past and not more than 1 hour in the
  future.
- Stop truncating the last character when reading *stats files.

The only thing that's left now is to avoid reading whole *stats files into
memory.

diff --git a/src/or/geoip.c b/src/or/geoip.c
index 622b582173504c9d48187a809c5ca78e429599ce..247e16cff312428e92af0dbfccc86a05d3d05206 100644 (file)
--- a/src/or/geoip.c
+++ b/src/or/geoip.c
@@ -365,6 +365,23 @@ geoip_get_mean_shares(time_t now, double *v2_share_out,
   return 0;
 }
 
+/* Rotate period of v2 and v3 network status requests. */
+static void
+rotate_request_period(void)
+{
+  SMARTLIST_FOREACH(geoip_countries, geoip_country_t *, c, {
+      memmove(&c->n_v2_ns_requests[0], &c->n_v2_ns_requests[1],
+              sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
+      memmove(&c->n_v3_ns_requests[0], &c->n_v3_ns_requests[1],
+              sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
+      c->n_v2_ns_requests[REQUEST_HIST_LEN-1] = 0;
+      c->n_v3_ns_requests[REQUEST_HIST_LEN-1] = 0;
+    });
+  current_request_period_starts += REQUEST_HIST_PERIOD;
+  if (n_old_request_periods < REQUEST_HIST_LEN-1)
+    ++n_old_request_periods;
+}
+
 /** Note that we've seen a client connect from the IP <b>addr</b> (host order)
  * at time <b>now</b>. Ignored by all but bridges and directories if
  * configured accordingly. */
@@ -404,17 +421,7 @@ geoip_note_client_seen(geoip_client_action_t action,
        * with action GEOIP_CLIENT_NETWORKSTATUS{_V2}.) */
       geoip_remove_old_clients(current_request_period_starts);
       /* Now rotate request period */
-      SMARTLIST_FOREACH(geoip_countries, geoip_country_t *, c, {
-          memmove(&c->n_v2_ns_requests[0], &c->n_v2_ns_requests[1],
-                  sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
-          memmove(&c->n_v3_ns_requests[0], &c->n_v3_ns_requests[1],
-                  sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
-          c->n_v2_ns_requests[REQUEST_HIST_LEN-1] = 0;
-          c->n_v3_ns_requests[REQUEST_HIST_LEN-1] = 0;
-        });
-      current_request_period_starts += REQUEST_HIST_PERIOD;
-      if (n_old_request_periods < REQUEST_HIST_LEN-1)
-        ++n_old_request_periods;
+      rotate_request_period();
     }
   }
 
@@ -1046,17 +1053,7 @@ geoip_dirreq_stats_write(time_t now)
   open_file = NULL;
 
   /* Rotate request period */
-  SMARTLIST_FOREACH(geoip_countries, geoip_country_t *, c, {
-      memmove(&c->n_v2_ns_requests[0], &c->n_v2_ns_requests[1],
-              sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
-      memmove(&c->n_v3_ns_requests[0], &c->n_v3_ns_requests[1],
-              sizeof(uint32_t)*(REQUEST_HIST_LEN-1));
-      c->n_v2_ns_requests[REQUEST_HIST_LEN-1] = 0;
-      c->n_v3_ns_requests[REQUEST_HIST_LEN-1] = 0;
-    });
-  current_request_period_starts += REQUEST_HIST_PERIOD;
-  if (n_old_request_periods < REQUEST_HIST_LEN-1)
-    ++n_old_request_periods;
+  rotate_request_period();
 
   start_of_dirreq_stats_interval = now;
 

diff --git a/src/or/relay.c b/src/or/relay.c
index a79a4c161af355036c80ba72171b59b53b1010ce..c81b8311a0ef56145a5290adba15e49e84c18d26 100644 (file)
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -1677,7 +1677,7 @@ cell_queue_clear(cell_queue_t *queue)
       queue->insertion_times->first = elem->next;
       mp_pool_release(elem);
     }
-    queue->insertion_times = NULL;
+    tor_free(queue->insertion_times);
   }
 }
 

diff --git a/src/or/rephist.c b/src/or/rephist.c
index f7d0b2bbcf629ca4e8bc5ead9f438764e2598a91..8d78ac26c35ac9d3d8855cc33ca42dca103d5854 100644 (file)
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -1371,6 +1371,9 @@ rep_hist_exit_stats_write(time_t now)
   open_file_t *open_file = NULL;
   FILE *out = NULL;
 
+  if (!exit_streams)
+    return; /* Not initialized */
+
   statsdir = get_datadir_fname("stats");
   if (check_private_dir(statsdir, CPD_CREATE) < 0)
     goto done;
@@ -1480,6 +1483,8 @@ rep_hist_note_exit_bytes_written(uint16_t port, size_t num_bytes)
 {
   if (!get_options()->ExitPortStatistics)
     return;
+  if (!exit_bytes_written)
+    return; /* Not initialized */
   exit_bytes_written[port] += num_bytes;
   log_debug(LD_HIST, "Written %lu bytes to exit connection to port %d.",
             (unsigned long)num_bytes, port);
@@ -1492,6 +1497,8 @@ rep_hist_note_exit_bytes_read(uint16_t port, size_t num_bytes)
 {
   if (!get_options()->ExitPortStatistics)
     return;
+  if (!exit_bytes_read)
+    return; /* Not initialized */
   exit_bytes_read[port] += num_bytes;
   log_debug(LD_HIST, "Read %lu bytes from exit connection to port %d.",
             (unsigned long)num_bytes, port);
@@ -1503,6 +1510,8 @@ rep_hist_note_exit_stream_opened(uint16_t port)
 {
   if (!get_options()->ExitPortStatistics)
     return;
+  if (!exit_streams)
+    return; /* Not initialized */
   exit_streams[port]++;
   log_debug(LD_HIST, "Opened exit stream to port %d", port);
 }

diff --git a/src/or/router.c b/src/or/router.c
index 64267f242c5dea9a1ab2cdd3fdcb766881106161..2d6734ae0446d80c466da3988956635eb94675d2 100644 (file)
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -1826,36 +1826,40 @@ router_dump_router_to_string(char *s, size_t maxlen, routerinfo_t *router,
   return (int)written+1;
 }
 
-/** Load the contents of <b>filename</b>, find the first line starting
- * with <b>end_line</b> that has a timestamp after <b>after</b>, and write
- * the file contents starting with that line to **<b>contents</b>. Return
- * 1 for success, 0 if the file does not exist or does not contain a line
- * matching these criteria, or -1 for failure. */
+/** Load the contents of <b>filename</b>, find the last line starting with
+ * <b>end_line</b>, ensure that its timestamp is not more than 25 hours in
+ * the past or more than 1 hour in the future with respect to <b>now</b>,
+ * and write the file contents starting with that line to **<b>out</b>.
+ * Return 1 for success, 0 if the file does not exist or does not contain
+ * a line matching these criteria, or -1 for failure. */
 static int
-load_stats_file(const char *filename, const char *end_line, time_t after,
+load_stats_file(const char *filename, const char *end_line, time_t now,
                 char **out)
 {
   int r = -1;
   char *fname = get_datadir_fname(filename);
-  char *contents, *start, timestr[ISO_TIME_LEN+1];
+  char *contents, *start = NULL, *tmp, timestr[ISO_TIME_LEN+1];
   time_t written;
   switch (file_status(fname)) {
     case FN_FILE:
+      /* X022 Find an alternative to reading the whole file to memory. */
       if ((contents = read_file_to_str(fname, 0, NULL))) {
-        start = contents;
-        do {
-          if (start != contents)
-            start++; /* Avoid infinite loops. */
-          if (!(start = strstr(start, end_line)))
-            goto notfound;
-          if (strlen(start) < strlen(end_line) + 1 + sizeof(timestr))
-            goto notfound;
-          strlcpy(timestr, start + 1 + strlen(end_line), sizeof(timestr));
-          if (parse_iso_time(timestr, &written) < 0)
-            goto notfound;
-        } while (written <= after);
-        *out = tor_malloc(strlen(start));
-        strlcpy(*out, start, strlen(start));
+        tmp = strstr(contents, end_line);
+        /* Find last block starting with end_line */
+        while (tmp) {
+          start = tmp;
+          tmp = strstr(tmp + 1, end_line);
+        }
+        if (!start)
+          goto notfound;
+        if (strlen(start) < strlen(end_line) + 1 + sizeof(timestr))
+          goto notfound;
+        strlcpy(timestr, start + 1 + strlen(end_line), sizeof(timestr));
+        if (parse_iso_time(timestr, &written) < 0)
+          goto notfound;
+        if (written < now - (25*60*60) || written > now + (1*60*60))
+          goto notfound;
+        *out = tor_strdup(start);
         r = 1;
       }
      notfound:
@@ -1908,7 +1912,8 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo,
         load_stats_file("stats"PATH_SEPARATOR"dirreq-stats",
                         "dirreq-stats-end", since, &contents) > 0) {
       int pos = strlen(s);
-      if (tor_snprintf(s + pos, maxlen - strlen(s), "%s\n", contents) < 0) {
+      if (strlcpy(s + pos, contents, maxlen - strlen(s)) !=
+          strlen(contents)) {
         log_warn(LD_DIR, "Could not write dirreq-stats to extra-info "
                  "descriptor.");
         s[pos] = '\0';
@@ -1919,7 +1924,8 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo,
         load_stats_file("stats"PATH_SEPARATOR"entry-stats",
                         "entry-stats-end", since, &contents) > 0) {
       int pos = strlen(s);
-      if (tor_snprintf(s + pos, maxlen - strlen(s), "%s\n", contents) < 0) {
+      if (strlcpy(s + pos, contents, maxlen - strlen(s)) !=
+          strlen(contents)) {
         log_warn(LD_DIR, "Could not write entry-stats to extra-info "
                  "descriptor.");
         s[pos] = '\0';
@@ -1930,7 +1936,8 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo,
         load_stats_file("stats"PATH_SEPARATOR"buffer-stats",
                         "cell-stats-end", since, &contents) > 0) {
       int pos = strlen(s);
-      if (tor_snprintf(s + pos, maxlen - strlen(s), "%s\n", contents) < 0) {
+      if (strlcpy(s + pos, contents, maxlen - strlen(s)) !=
+          strlen(contents)) {
         log_warn(LD_DIR, "Could not write buffer-stats to extra-info "
                  "descriptor.");
         s[pos] = '\0';
@@ -1941,7 +1948,8 @@ extrainfo_dump_to_string(char *s, size_t maxlen, extrainfo_t *extrainfo,
         load_stats_file("stats"PATH_SEPARATOR"exit-stats",
                         "exit-stats-end", since, &contents) > 0) {
       int pos = strlen(s);
-      if (tor_snprintf(s + pos, maxlen - strlen(s), "%s\n", contents) < 0) {
+      if (strlcpy(s + pos, contents, maxlen - strlen(s)) !=
+          strlen(contents)) {
         log_warn(LD_DIR, "Could not write exit-stats to extra-info "
                  "descriptor.");
         s[pos] = '\0';