Resolve many XXXs and all DOCDOCs

svn:r2755

11 files changed, 138 insertions, 75 deletions

diff --git a/src/common/util.c b/src/common/util.c
index 8311f95be..d3c044db8 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -283,6 +283,18 @@ int strcmpstart(const char *s1, const char *s2)
   return strncmp(s1, s2, n);
 }
 
+/* Compares the last strlen(s2) characters of s1 with s2.  Returns as for
+ * strcmp.
+ */
+int strcmpend(const char *s1, const char *s2)
+{
+  size_t n1 = strlen(s1), n2 = strlen(s2);
+  if (n2>n1)
+    return strcmp(s1,s2);
+  else
+    return strncmp(s1+(n1-n2), s2, n2);
+}
+
 
 /** Return a pointer to the first char of s that is not whitespace and
  * not a comment, or to the terminating NUL if no such character exists.
@@ -554,7 +566,7 @@ static const char *MONTH_NAMES[] =
 void format_rfc1123_time(char *buf, time_t t) {
   struct tm *tm = gmtime(&t);
 
-  strftime(buf, RFC1123_TIME_LEN+1, "XXX, %d XXX %Y %H:%M:%S GMT", tm);
+  strftime(buf, RFC1123_TIME_LEN+1, "___, %d ___ %Y %H:%M:%S GMT", tm);
   tor_assert(tm->tm_wday >= 0);
   tor_assert(tm->tm_wday <= 6);
   memcpy(buf, WEEKDAY_NAMES[tm->tm_wday], 3);
@@ -732,7 +744,6 @@ int check_private_dir(const char *dirname, cpd_check_t check)
         return -1;
       }
     }
-
     /* XXXX In the case where check==CPD_CHECK, we should look at the
      * parent directory a little harder. */
     return 0;
@@ -771,7 +782,8 @@ write_str_to_file(const char *fname, const char *str, int bin)
   return write_bytes_to_file(fname, str, strlen(str), bin);
 }
 
-/* DOCDOC */
+/** As write_str_to_file, but does not assume a NUL-terminated *
+ * string. Instead, we write <b>len</b> bytes, starting at <b>str</b>. */
 int write_bytes_to_file(const char *fname, const char *str, size_t len,
                         int bin)
 {
@@ -799,30 +811,7 @@ int write_bytes_to_file(const char *fname, const char *str, size_t len,
     log(LOG_WARN,"Error flushing to %s: %s", tempname, strerror(errno));
     return -1;
   }
-
-  /* XXXX use replace_file() instead. */
-#ifdef MS_WINDOWS
-  /* On Windows, rename doesn't replace.  We could call ReplaceFile, but
-   * that's hard, and we can probably sneak by without atomicity. */
-  switch (file_status(fname)) {
-    case FN_ERROR:
-      log(LOG_WARN, "Error replacing %s: %s", fname, strerror(errno));
-      return -1;
-    case FN_DIR:
-      log(LOG_WARN, "Error replacing %s: is directory", fname);
-      return -1;
-    case FN_FILE:
-      if (unlink(fname)) {
-        log(LOG_WARN, "Error replacing %s while removing old copy: %s",
-            fname, strerror(errno));
-        return -1;
-      }
-      break;
-    case FN_NOENT:
-      ;
-  }
-#endif
-  if (rename(tempname, fname)) {
+  if (replace_file(tempname, fname)) {
     log(LOG_WARN, "Error replacing %s: %s", fname, strerror(errno));
     return -1;
   }
@@ -875,9 +864,15 @@ char *read_file_to_str(const char *filename, int bin) {
   return string;
 }
 
-/** DOCDOC.
+/** Given a string containing part of a configuration file or similar format,
+ * advance past comments and whitespace and try to parse a single line.  If we
+ * parse a line successfully, set *<b>key_out</b> to the key portion and
+ * *<b>value_out</b> to the value portion of the line, and return a pointer to
+ * the start of the next line.  If we run out of data, return a pointer to the
+ * end of the string.  If we encounter an error, return NULL.
  *
- * Return next line or end of string on success, NULL on failure.
+ * NOTE: We modify <b>line</b> as we parse it, by inserting NULs to terminate
+ * the key and value.
  */
 char *
 parse_line_from_str(char *line, char **key_out, char **value_out)
@@ -958,7 +953,9 @@ char *expand_filename(const char *filename)
      * Round up to 16 in case we can't do math. */
     len = strlen(home)+strlen(filename)+16;
     result = tor_malloc(len);
-    tor_snprintf(result,len,"%s/%s",home,filename+2);
+    tor_snprintf(result,len,"%s%s%s",home,
+                 (!strcmpend(home, "/")) ? "" : "/",
+                 filename+2);
     return result;
   } else {
     return tor_strdup(filename);
diff --git a/src/common/util.h b/src/common/util.h
index 9f116b579..043d77c45 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -47,6 +47,7 @@ char *tor_strndup(const char *s, size_t n);
 #define HEX_CHARACTERS "0123456789ABCDEFabcdef"
 void tor_strlower(char *s);
 int strcmpstart(const char *s1, const char *s2);
+int strcmpend(const char *s1, const char *s2);
 int tor_strstrip(char *s, const char *strip);
 typedef enum {
   ALWAYS_TERMINATE, NEVER_TERMINATE, TERMINATE_IF_EVEN
diff --git a/src/or/config.c b/src/or/config.c
index 847b30987..741f86f2b 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -165,7 +165,7 @@ static int options_transition_allowed(or_options_t *old, or_options_t *new);
 static int check_nickname_list(const char *lst, const char *name);
 
 static int parse_dir_server_line(const char *line, int validate_only);
-static int parse_redirect_line(or_options_t *options,
+static int parse_redirect_line(smartlist_t *result,
                                struct config_line_t *line);
 static int parse_log_severity_range(const char *range, int *min_out,
                                     int *max_out);
@@ -264,14 +264,27 @@ options_act(void) {
   close_temp_logs();
   add_callback_log(LOG_NOTICE, LOG_ERR, control_event_logmsg);
 
+  {
+    smartlist_t *sl = smartlist_create();
+    for (cl = options->RedirectExit; cl; cl = cl->next) {
+      if (parse_redirect_line(sl, cl)<0)
+        return -1;
+    }
+    set_exit_redirects(sl);
+  }
+
   /* Start backgrounding the process, if requested. */
+
+  /* XXXX We once had a reason to separate start_daemon and finish_daemon: It
+   *    let us have the parent process stick around until we were sure Tor was
+   *    started.  Should se make start_daemon get called earlier? -NM */
   if (options->RunAsDaemon) {
     start_daemon(options->DataDirectory);
   }
 
   /* Finish backgrounding the process */
   if(options->RunAsDaemon) {
-    /* XXXX Can we delay this any more? */
+    /* We may be calling this for the n'th time (on SIGHUP), but it's safe. */
     finish_daemon();
   }
 
@@ -599,7 +612,7 @@ config_get_assigned_option(or_options_t *options, const char *key)
       }
       break;
     case CONFIG_TYPE_UINT:
-      /* XXX This means every or_options_t uint or bool element
+      /* This means every or_options_t uint or bool element
        * needs to be an int. Not, say, a uint16_t or char. */
       tor_snprintf(buf, sizeof(buf), "%d", *(int*)value);
       result->value = tor_strdup(buf);
@@ -916,13 +929,6 @@ options_free(or_options_t *options)
         break;
     }
   }
-  /* XXX this last part is an exception. can we make it not needed? */
-  if (options->RedirectExitList) {
-    SMARTLIST_FOREACH(options->RedirectExitList,
-                      exit_redirect_t *, p, tor_free(p));
-    smartlist_free(options->RedirectExitList);
-    options->RedirectExitList = NULL;
-  }
 }
 
 /** Copy storage held by <b>old</b> into a new or_options_t and return it. */
@@ -1180,16 +1186,8 @@ options_validate(or_options_t *options)
       result = -1;
   }
 
-  /* free options->RedirectExitList */
-  if (options->RedirectExitList) {
-    SMARTLIST_FOREACH(options->RedirectExitList,
-                      exit_redirect_t *, p, tor_free(p));
-    smartlist_free(options->RedirectExitList);
-  }
-
-  options->RedirectExitList = smartlist_create();
   for (cl = options->RedirectExit; cl; cl = cl->next) {
-    if (parse_redirect_line(options, cl)<0)
+    if (parse_redirect_line(NULL, cl)<0)
       result = -1;
   }
 
@@ -1729,17 +1727,16 @@ exit_policy_free(struct exit_policy_t *p) {
   }
 }
 
-/** Parse a single RedirectExit line's contents from <b>line</b>.  If they are
- *  valid, add an element to <b>options</b>-&gt;RedirectExitList and return 0.
+/** Parse a single RedirectExit line's contents from <b>line</b>.  If
+ *  they are valid, and <b>result</b> is not NULL, add an element to
+ *  <b>result</b> and return 0. Else if they are valid, return 0.
  *  Else return -1. */
 static int
-parse_redirect_line(or_options_t *options, struct config_line_t *line)
+parse_redirect_line(smartlist_t *result, struct config_line_t *line)
 {
   smartlist_t *elements = NULL;
   exit_redirect_t *r;
 
-  tor_assert(options);
-  tor_assert(options->RedirectExitList);
   tor_assert(line);
 
   r = tor_malloc_zero(sizeof(exit_redirect_t));
@@ -1773,7 +1770,10 @@ parse_redirect_line(or_options_t *options, struct config_line_t *line)
   SMARTLIST_FOREACH(elements, char *, cp, tor_free(cp));
   smartlist_free(elements);
   if (r) {
-    smartlist_add(options->RedirectExitList, r);
+    if (result)
+      smartlist_add(result, r);
+    else
+      tor_free(r);
     return 0;
   } else {
     return -1;
@@ -1840,6 +1840,9 @@ parse_dir_server_line(const char *line, int validate_only)
   return r;
 }
 
+
+/** Adjust or the value of options->DataDirectory, or fill it in if it's
+ * absent. Return 0 on success, -1 on failure. */
 static int
 normalize_data_directory(or_options_t *options) {
 #ifdef MS_WINDOWS
@@ -1861,6 +1864,11 @@ normalize_data_directory(or_options_t *options) {
      log_fn(LOG_ERR,"Failed to expand filename '%s'.", d);
      return -1;
    }
+   if (!options->DataDirectory && !strcmp(fn,"/.tor")) {
+     /* If our homedir is /, we probably don't want to use it. */
+     /* XXXX Default to /var/lib/tor? */
+     log_fn(LOG_WARN, "Defaulting to %s, which may not be what you want", fn);
+   }
    tor_free(options->DataDirectory);
    options->DataDirectory = fn;
  }
@@ -1868,6 +1876,8 @@ normalize_data_directory(or_options_t *options) {
 #endif
 }
 
+/** Check and normalize the value of options->DataDirectory; return 0 if it
+ * sane, -1 otherwise. */
 static int
 validate_data_directory(or_options_t *options) {
   if (normalize_data_directory(options) < 0)
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index e679fbf97..2a4cd4f1b 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -13,6 +13,8 @@
 #include "tree.h"
 
 static struct exit_policy_t *socks_policy = NULL;
+/* List of exit_redirect_t */
+static smartlist_t *redirect_exit_list = NULL;
 
 static int connection_ap_handshake_process_socks(connection_t *conn);
 static void parse_socks_policy(void);
@@ -896,7 +898,8 @@ int connection_exit_begin_resolve(cell_t *cell, circuit_t *circ) {
  * address, but <em>only</em> if it's a general exit stream. (Rendezvous
  * streams must not reveal what IP they connected to.)
  */
-void connection_exit_connect(connection_t *conn) {
+void
+connection_exit_connect(connection_t *conn) {
   unsigned char connected_payload[4];
   uint32_t addr;
   uint16_t port;
@@ -913,7 +916,8 @@ void connection_exit_connect(connection_t *conn) {
 
   addr = conn->addr;
   port = conn->port;
-  SMARTLIST_FOREACH(options->RedirectExitList, exit_redirect_t *, r,
+  if (redirect_exit_list) {
+    SMARTLIST_FOREACH(redirect_exit_list, exit_redirect_t *, r,
     {
       if ((addr&r->mask)==(r->addr&r->mask) &&
           (r->port_min <= port) && (port <= r->port_max)) {
@@ -928,6 +932,7 @@ void connection_exit_connect(connection_t *conn) {
         break;
       }
     });
+  }
 
   log_fn(LOG_DEBUG,"about to try connecting");
   switch(connection_connect(conn, conn->address, addr, port)) {
@@ -1198,6 +1203,19 @@ void client_dns_clean(void)
   strmap_foreach(client_dns_map, (strmap_foreach_fn)_remove_if_expired, &now);
 }
 
+
+/** Make connection redirection follow the provided list of
+ * exit_redirect_t */
+void
+set_exit_redirects(smartlist_t *lst)
+{
+  if (redirect_exit_list) {
+    SMARTLIST_FOREACH(redirect_exit_list, exit_redirect_t *, p, tor_free(p));
+    smartlist_free(redirect_exit_list);
+  }
+  redirect_exit_list = lst;
+}
+
 /*
   Local Variables:
   mode:c
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 8e3cc5f79..9a11f0bdb 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -119,6 +119,14 @@ dirserv_parse_fingerprint_file(const char *fname)
              nickname, fingerprint);
       continue;
     }
+    if (0==strcasecmp(ent->nickname, DEFAULT_CLIENT_NICKNAME)) {
+      /* If you approved an OR called "client", then clients who use
+       * the default nickname could all be rejected.  That's no good. */
+      log(LOG_WARN,
+          "Authorizing a nickname '%s' would break many clients; skipping.",
+          DEFAULT_CLIENT_NICKNAME);
+      continue;
+    }
     for (i = 0; i < smartlist_len(fingerprint_list_new); ++i) {
       ent = smartlist_get(fingerprint_list_new, i);
       if (0==strcasecmp(ent->nickname, nickname)) {
@@ -388,8 +396,7 @@ dirserv_add_descriptor(const char **desc)
   ent->desc_len = desc_len;
   ent->descriptor = tor_strndup(start,desc_len);
   ent->router = ri;
-  /* XXX008 is ent->verified useful/used for anything? */
-  ent->verified = verified; /* XXXX008 support other possibilities. */
+  ent->verified = verified;
   smartlist_add(descriptor_list, ent);
 
   *desc = end;
@@ -692,7 +699,9 @@ static char *cached_directory_z = NULL;
 static size_t cached_directory_z_len = 0;
 static time_t cached_directory_published = 0;
 
-/** DOCDOC */
+/** If we have no cached directory, or it is older than <b>when</b>, then
+ * replace it with <b>directory</b>, published at <b>when</b>.
+ */
 void dirserv_set_cached_directory(const char *directory, time_t when)
 {
   time_t now;
diff --git a/src/or/main.c b/src/or/main.c
index 0f0da41ee..4a2e9cc08 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -1058,7 +1058,8 @@ static void do_list_fingerprint(void)
   printf("%s %s\n", nickname?nickname:"client", buf);
 }
 
-/** DOCDOC **/
+/** Entry point for password hashing: take the desired password from
+ * the command line, and print its salted hash to stdout. **/
 static void do_hash_password(void)
 {
 
diff --git a/src/or/or.h b/src/or/or.h
index 221d2b36a..4ab2d23ce 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -432,6 +432,9 @@
 /* legal characters in a nickname */
 #define LEGAL_NICKNAME_CHARACTERS "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
 
+/** Name to use in client TLS certificates if no nickname is given.*/
+#define DEFAULT_CLIENT_NICKNAME "client"
+
 #define SOCKS4_NETWORK_LEN 8
 
 /*
@@ -1210,6 +1213,7 @@ uint32_t client_dns_lookup_entry(const char *address);
 int client_dns_incr_failures(const char *address);
 void client_dns_set_entry(const char *address, uint32_t val);
 void client_dns_clean(void);
+void set_exit_redirects(smartlist_t *lst);
 
 /********************************* connection_or.c ***************************/
 
diff --git a/src/or/rephist.c b/src/or/rephist.c
index c49dd62e3..94cd11cd2 100644
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -511,9 +511,11 @@ int rep_hist_bandwidth_assess(void) {
 }
 
 /**
- * DOCDOC
+ * Allocate and return lines for representing this server's bandwidth
+ * history in its descriptor.
  */
-char *rep_hist_get_bandwidth_lines(void)
+char *
+rep_hist_get_bandwidth_lines(void)
 {
   char *buf, *cp;
   char t[ISO_TIME_LEN+1];
@@ -521,7 +523,7 @@ char *rep_hist_get_bandwidth_lines(void)
   bw_array_t *b;
   size_t len;
 
-  /* opt (read|write)-history yyyy-mm-dd HH:MM:SS (xxx s) n,n,n,n,n... */
+  /* opt (read|write)-history yyyy-mm-dd HH:MM:SS (n s) n,n,n,n,n... */
   len = (60+12*NUM_TOTALS)*2;
   buf = tor_malloc_zero(len);
   cp = buf;
diff --git a/src/or/router.c b/src/or/router.c
index c20cc96dd..bae0a57d9 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -247,8 +247,7 @@ int init_keys(void) {
     if (crypto_pk_generate_key(prkey))
       return -1;
     set_identity_key(prkey);
-    /* XXX NM: do we have a convention for what client's Nickname is?
-     * No.  Let me propose one: */
+    /* Create a TLS context; default the client nickname to "client". */
     if (tor_tls_context_new(get_identity_key(), 1,
                             options->Nickname ? options->Nickname : "client",
                             MAX_SSL_KEY_LIFETIME) < 0) {
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index 27e59ea9f..59b4fac27 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -345,12 +345,6 @@ router_parse_routerlist_from_directory(const char *str,
   smartlist_free(tokens);
   tokens = NULL;
 
-  if(!get_options()->AuthoritativeDir) {
-    /* Now that we know the signature is okay, cache the directory. */
-    /* XXXX009 extract published time if possible. */
-    dirserv_set_cached_directory(str, time(NULL));
-  }
-
   /* Now that we know the signature is okay, check the version. */
   if (check_version)
     check_software_version_against_directory(str, get_options()->IgnoreVersion);
@@ -393,6 +387,12 @@ router_parse_routerlist_from_directory(const char *str,
      goto err;
   }
 
+  if(!get_options()->AuthoritativeDir) {
+    /* Now that we know the signature is okay, and we have a
+     * publication time, cache the directory. */
+    dirserv_set_cached_directory(str, published_on);
+  }
+
   if (!(tok = find_first_by_keyword(tokens, K_RECOMMENDED_SOFTWARE))) {
     log_fn(LOG_WARN, "Missing recommended-software line from directory.");
     goto err;
@@ -863,14 +863,22 @@ routerinfo_t *router_parse_entry_from_string(const char *s,
   if (!(tok = find_first_by_keyword(tokens, K_ONION_KEY))) {
     log_fn(LOG_WARN, "Missing onion key"); goto err;
   }
-  /* XXX Check key length */
+  if (crypto_pk_keysize(tok->key) != PK_BYTES) {
+    log_fn(LOG_WARN, "Wrong size on onion key: %d bits!",
+           crypto_pk_keysize(tok->key)*8);
+    goto err;
+  }
   router->onion_pkey = tok->key;
   tok->key = NULL; /* Prevent free */
 
   if (!(tok = find_first_by_keyword(tokens, K_SIGNING_KEY))) {
     log_fn(LOG_WARN, "Missing identity key"); goto err;
   }
-  /* XXX Check key length */
+  if (crypto_pk_keysize(tok->key) != PK_BYTES) {
+    log_fn(LOG_WARN, "Wrong size on identity key: %d bits!",
+           crypto_pk_keysize(tok->key)*8);
+    goto err;
+  }
   router->identity_pkey = tok->key;
   tok->key = NULL; /* Prevent free */
   if (crypto_pk_get_digest(router->identity_pkey,router->identity_digest)){
@@ -1420,7 +1428,8 @@ int tor_version_as_new_as(const char *platform, const char *cutoff) {
   return tor_version_compare(&router_version, &cutoff_version) >= 0;
 }
 
-/** DOCDOC */
+/** Parse a tor version from <b>s</b>, and store the result in <b>out</b>.
+ * Return 0 on success, -1 on failure. */
 int tor_version_parse(const char *s, tor_version_t *out)
 {
   char *eos=NULL, *cp=NULL;
diff --git a/src/or/test.c b/src/or/test.c
index a8d9f63cc..be088e5f8 100644
--- a/src/or/test.c
+++ b/src/or/test.c
@@ -727,6 +727,19 @@ test_util(void) {
   test_streq(v, "val");
   test_streq(cp, "");
 
+  /* Test for strcmpstart and strcmpend. */
+  test_assert(strcmpstart("abcdef", "abcdef")==0);
+  test_assert(strcmpstart("abcdef", "abc")==0);
+  test_assert(strcmpstart("abcdef", "abd")<0);
+  test_assert(strcmpstart("abcdef", "abb")>0);
+  test_assert(strcmpstart("ab", "abb")<0);
+
+  test_assert(strcmpend("abcdef", "abcdef")==0);
+  test_assert(strcmpend("abcdef", "def")==0);
+  test_assert(strcmpend("abcdef", "deg")<0);
+  test_assert(strcmpend("abcdef", "dee")>0);
+  test_assert(strcmpend("ab", "abb")<0);
+
   /* XXXX test older functions. */
   smartlist_free(sl);
 }