r14063@tombo: nickm | 2008-02-08 15:48:32 -0500

 Add a bunch more code documentation; change the interface of fetch_var_cell_from_buf() so it takes the current link protocol into account and can't get confused by weird command bytes on v1 connections.


svn:r13430

8 files changed, 81 insertions, 36 deletions

diff --git a/ChangeLog b/ChangeLog
index ec70469da..1957541e1 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -59,6 +59,8 @@ Changes in version 0.2.0.19-alpha - 2008-02-??
       on 0.1.2.x.
     - Fix shell error when warning about missing packages in configure
       script, on fedora or redhat machines. Bugfix on 0.2.0.x.
+    - Do not become confused when receiving a spurious VERSIONS-like
+      cell from a confused v1 client.  Bugfix on 0.2.0.x.
 
   o Code simplifications and refactoring:
     - Remove some needless generality from cpuworker code, for improved
diff --git a/src/or/buffers.c b/src/or/buffers.c
index a1a6baab4..ad855b147 100644
--- a/src/or/buffers.c
+++ b/src/or/buffers.c
@@ -727,7 +727,12 @@ flush_chunk(int s, buf_t *buf, chunk_t *chunk, size_t sz,
   }
 }
 
-/** DOCDOC */
+/** Helper for flush_buf_tls(): try to write <b>sz</b> bytes from chunk
+ * <b>chunk</b> of buffer <b>buf</b> onto socket <b>s</b>.  (Tries to write
+ * more if there is a forced pending write size.)  On success, deduct the
+ * bytes written from *<b>buf_flushlen</b>.  Return the number of bytes
+ * written on success, and a TOR_TLS error code on failue or blocking.
+ */
 static INLINE int
 flush_chunk_tls(tor_tls_t *tls, buf_t *buf, chunk_t *chunk,
                 size_t sz, size_t *buf_flushlen)
@@ -799,8 +804,8 @@ flush_buf(int s, buf_t *buf, size_t sz, size_t *buf_flushlen)
   return flushed;
 }
 
-/** As flush_buf(), but writes data to a TLS connection.
- * DOCDOC can write more than flushlen bytes.
+/** As flush_buf(), but writes data to a TLS connection.  Can write more than
+ * <b>flushlen</b> bytes.
  */
 int
 flush_buf_tls(tor_tls_t *tls, buf_t *buf, size_t flushlen,
@@ -918,15 +923,26 @@ fetch_from_buf(char *string, size_t string_len, buf_t *buf)
   return buf->datalen;
 }
 
-/** DOCDOC Returns 0 on "not a var-length cell."; 1 whether it's all here
- * yet or not. */
+/** Check <b>buf</b> for a variable-length cell according to the rules of link
+ * protocol version <b>linkproto</b>.  If one is found, pull it off the buffer
+ * and assign a newly allocated var_cell_t to *<b>out</b>, and return 1.
+ * Return 0 if whatever is on the start of buf_t is not a variable-length
+ * cell.  Return 1 and set *<b>out</b> to NULL if there seems to be the start
+ * of a variable-length cell on <b>buf</b>, but the whole thing isn't there
+ * yet. */
 int
-fetch_var_cell_from_buf(buf_t *buf, var_cell_t **out)
+fetch_var_cell_from_buf(buf_t *buf, var_cell_t **out, int linkproto)
 {
   char hdr[VAR_CELL_HEADER_SIZE];
   var_cell_t *result;
   uint8_t command;
   uint16_t length;
+  /* If linkproto is unknown (0) or v2 (2), variable-length cells work as
+   * implemented here. If it's 1, there are no variable-length cells.  Tor
+   * does not support other versions right now, and so can't negotiate them.
+   */
+  if (linkproto == 1)
+    return 0;
   check();
   *out = NULL;
   if (buf->datalen < VAR_CELL_HEADER_SIZE)
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 6ba91fa39..c295d8aad 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -2822,7 +2822,7 @@ getinfo_helper_entry_guards(control_connection_t *conn,
   return 0;
 }
 
-/** DOCDOC */
+/** DOCDOC arma */
 typedef struct {
   uint32_t addr;
   uint16_t port;
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index c3bb69ec1..8f38b615f 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -1994,8 +1994,6 @@ connection_ap_handshake_send_resolve(edge_connection_t *ap_conn)
  * and call connection_ap_handshake_attach_circuit(conn) on it.
  *
  * Return the other end of the linked connection pair, or -1 if error.
- *
- * DOCDOC start_reading
  */
 edge_connection_t *
 connection_ap_make_link(char *address, uint16_t port,
@@ -2051,7 +2049,9 @@ connection_ap_make_link(char *address, uint16_t port,
   return conn;
 }
 
-/** DOCDOC */
+/** Notify any interested controller connections about a new hostname resolve
+ * or resolve error.  Takes the same arguments as does
+ * connection_ap_handshake_socks_resolved(). */
 static void
 tell_controller_about_resolved_result(edge_connection_t *conn,
                                       int answer_type,
@@ -2088,7 +2088,7 @@ tell_controller_about_resolved_result(edge_connection_t *conn,
  * via SOCKS.  The type should be one of RESOLVED_TYPE_(IPV4|IPV6|HOSTNAME) or
  * -1 for unreachable; the answer should be in the format specified
  * in the socks extensions document.
- * DOCDOC expires
+ * DOCDOC ttl expires
  **/
 void
 connection_ap_handshake_socks_resolved(edge_connection_t *conn,
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index 396e3914b..de6f58cd0 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -152,7 +152,8 @@ cell_unpack(cell_t *dest, const char *src)
   memcpy(dest->payload, src+3, CELL_PAYLOAD_SIZE);
 }
 
-/** DOCDOC */
+/** Write the header of <b>cell</b> into the first VAR_CELL_HEADER_SIZE
+ * bytes of <b>hdr_out</b>. */
 void
 var_cell_pack_header(const var_cell_t *cell, char *hdr_out)
 {
@@ -161,7 +162,8 @@ var_cell_pack_header(const var_cell_t *cell, char *hdr_out)
   set_uint16(hdr_out+3, htons(cell->payload_len));
 }
 
-/* DOCDOC*/
+/** Allocate and return a new var_cell_t with <b>payload_len</b> bytes of
+ * payload space. */
 var_cell_t *
 var_cell_new(uint16_t payload_len)
 {
@@ -172,7 +174,7 @@ var_cell_new(uint16_t payload_len)
   return cell;
 }
 
-/** DOCDOC */
+/** Release all space held by <b>cell</b> */
 void
 var_cell_free(var_cell_t *cell)
 {
@@ -579,13 +581,17 @@ connection_tls_start_handshake(or_connection_t *conn, int receiving)
   return 0;
 }
 
-/*DOCDOC*/
+/** Invoked on the server side from inside tor_tls_read() when the server
+ * gets a successful TLS renegotiation from the client. */
 static void
 connection_or_tls_renegotiated_cb(tor_tls_t *tls, void *_conn)
 {
   or_connection_t *conn = _conn;
   (void)tls;
 
+  /* Don't invoke this again. */
+  tor_tls_set_renegotiate_callback(tls, NULL, NULL);
+
   if (connection_tls_finish_handshake(conn) < 0) {
     /* XXXX_TLS double-check that it's ok to do this from inside read. */
     /* XXXX_TLS double-check that this verifies certificates. */
@@ -937,7 +943,10 @@ connection_or_write_cell_to_buf(const cell_t *cell, or_connection_t *conn)
     conn->timestamp_last_added_nonpadding = time(NULL);
 }
 
-/**DOCDOC*/
+/** Pack a variable-length <b>cell</b> into wire-format, and write it onto
+ * <b>conn</b>'s outbuf.  Right now, this <em>DOES NOT</em> support cells that
+ * affect a circuit.
+ */
 void
 connection_or_write_var_cell_to_buf(const var_cell_t *cell,
                                     or_connection_t *conn)
@@ -952,11 +961,12 @@ connection_or_write_var_cell_to_buf(const var_cell_t *cell,
     conn->timestamp_last_added_nonpadding = time(NULL);
 }
 
-/** DOCDOC */
+/** See whether there's a variable-length cell waiting on <b>conn</b>'s
+ * inbuf.  Return values as for fetch_var_cell_from_buf(). */
 static int
 connection_fetch_var_cell_from_buf(or_connection_t *conn, var_cell_t **out)
 {
-  return fetch_var_cell_from_buf(conn->_base.inbuf, out);
+  return fetch_var_cell_from_buf(conn->_base.inbuf, out, conn->link_proto);
 }
 
 /** Process cells from <b>conn</b>'s inbuf.
@@ -1026,12 +1036,14 @@ connection_or_send_destroy(uint16_t circ_id, or_connection_t *conn, int reason)
   return 0;
 }
 
-/**DOCDOC*/
+/** Array of recognized link protocol versions. */
 static const uint16_t or_protocol_versions[] = { 1, 2 };
+/** Number of versions in <b>or_protocol_versions</b>. */
 static const int n_or_protocol_versions =
   sizeof(or_protocol_versions)/sizeof(uint16_t);
 
-/**DOCDOC*/
+/** Return true iff <b>v</b> is a link protocol version that this Tor
+ * implementation believes it can support. */
 int
 is_or_protocol_version_known(uint16_t v)
 {
@@ -1043,7 +1055,8 @@ is_or_protocol_version_known(uint16_t v)
   return 0;
 }
 
-/** DOCDOC */
+/** Send a VERSIONS cell on <b>conn</b>, telling the other host about the
+ * link protocol versions that this Tor can support. */
 static int
 connection_or_send_versions(or_connection_t *conn)
 {
@@ -1065,7 +1078,8 @@ connection_or_send_versions(or_connection_t *conn)
   return 0;
 }
 
-/** DOCDOC */
+/** Send a NETINFO cell on <b>conn</b>, telling the other server what we know
+ * about their address, our address, and the current time. */
 int
 connection_or_send_netinfo(or_connection_t *conn)
 {
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 729e79f59..972b16e0f 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -784,7 +784,9 @@ networkstatus_vote_find_entry(networkstatus_t *ns, const char *digest)
 }
 
 /*XXXX make this static once functions are moved into this file. */
-/** DOCDOC */
+/** Search the routerstatuses in <b>ns</b> for one whose identity digest is
+ * <b>digest</b>.  Return value and set *<b>found_out</b> as for
+ * smartlist_besearch_idx(). */
 int
 networkstatus_vote_find_entry_idx(networkstatus_t *ns,
                                   const char *digest, int *found_out)
@@ -944,7 +946,8 @@ networkstatus_get_router_digest_by_nickname(const char *nickname)
   return strmap_get_lc(named_server_map, nickname);
 }
 
-/** DOCDOC */
+/** Return true iff <b>nickname</b> is disallowed from being the nickname
+ * of any server. */
 int
 networkstatus_nickname_is_unnamed(const char *nickname)
 {
@@ -1020,9 +1023,10 @@ update_v2_networkstatus_cache_downloads(time_t now)
   }
 }
 
-/**DOCDOC*/
+/** How many times will we try to fetch a consensus before we give up? */
 #define CONSENSUS_NETWORKSTATUS_MAX_DL_TRIES 8
-/**DOCDOC*/
+/** How long will we hang onto a possibly live consensus for which we're
+ * fetching certs before we check whether there is a better one? */
 #define DELAY_WHILE_FETCHING_CERTS (20*60)
 
 /** If we want to download a fresh consensus, launch a new download as
@@ -1045,6 +1049,7 @@ update_consensus_networkstatus_downloads(time_t now)
     return; /* There's an in-progress download.*/
 
   if (consensus_waiting_for_certs) {
+    /* XXXX020 make sure this doesn't delay sane downloads. */
     if (consensus_waiting_for_certs_set_at + DELAY_WHILE_FETCHING_CERTS > now)
       return; /* We're still getting certs for this one. */
     else {
@@ -1070,7 +1075,8 @@ networkstatus_consensus_download_failed(int status_code)
   update_consensus_networkstatus_downloads(time(NULL));
 }
 
-/**DOCDOC*/
+/** How long do we (as a cache) wait after a consensus becomes non-fresh
+ * before trying to fetch another? */
 #define CONSENSUS_MIN_SECONDS_BEFORE_CACHING 120
 
 /** Update the time at which we'll consider replacing the current
@@ -1160,7 +1166,8 @@ update_networkstatus_downloads(time_t now)
   update_certificate_downloads(now);
 }
 
-/**DOCDOC */
+/** Launch requests as appropriate for missing directory authority
+ * certificates. */
 void
 update_certificate_downloads(time_t now)
 {
@@ -1206,7 +1213,8 @@ networkstatus_get_live_consensus(time_t now)
 /* XXXX020 remove this in favor of get_live_consensus. But actually,
  * leave something like it for bridge users, who need to not totally
  * lose if they spend a while fetching a new consensus. */
-/** DOCDOC */
+/** As networkstatus_get_live_consensus(), but is way more tolerant of expired
+ * consensuses. */
 networkstatus_t *
 networkstatus_get_reasonably_live_consensus(time_t now)
 {
@@ -1751,7 +1759,8 @@ routers_update_status_from_consensus_networkstatus(smartlist_t *routers,
   router_dir_info_changed();
 }
 
-/**DOCDOC*/
+/** Given a list of signed_descriptor_t, update their fields (mainly, when
+ * they were last listed) from the most recent consensus. */
 void
 signed_descs_update_status_from_consensus_networkstatus(smartlist_t *descs)
 {
diff --git a/src/or/or.h b/src/or/or.h
index 7d1904b99..50c2c9337 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -245,14 +245,17 @@ typedef enum {
 #define OR_CONN_STATE_PROXY_FLUSHING 2
 /** State for a connection to an OR: waiting for proxy response. */
 #define OR_CONN_STATE_PROXY_READING 3
-/** State for a connection to an OR: SSL is handshaking, not done yet. */
+/** State for a connection to an OR or client: SSL is handshaking, not done
+ * yet. */
 #define OR_CONN_STATE_TLS_HANDSHAKING 4
-/** DOCDOC */
+/** State for a connection to an OR: We're doing a second SSL handshake for
+ * renegotiation purposes. */
 #define OR_CONN_STATE_TLS_CLIENT_RENEGOTIATING 5
-/** DOCDOC */
+/** State for a connection at an OR: We're waiting for the client to
+ * renegotiate. */
 #define OR_CONN_STATE_TLS_SERVER_RENEGOTIATING 6
 /** State for a connection to an OR: We're done with our SSL handshake, but we
- * haven't yet negotiated link protocol versions and finished authenticating.
+ * haven't yet negotiated link protocol versions and sent a netinfo cell.
  */
 #define OR_CONN_STATE_OR_HANDSHAKING 7
 /** State for a connection to an OR: Ready to send/receive cells. */
@@ -2457,7 +2460,7 @@ int write_to_buf_zlib(buf_t *buf, tor_zlib_state_t *state,
                       const char *data, size_t data_len, int done);
 int move_buf_to_buf(buf_t *buf_out, buf_t *buf_in, size_t *buf_flushlen);
 int fetch_from_buf(char *string, size_t string_len, buf_t *buf);
-int fetch_var_cell_from_buf(buf_t *buf, var_cell_t **out);
+int fetch_var_cell_from_buf(buf_t *buf, var_cell_t **out, int linkproto);
 int fetch_from_buf_http(buf_t *buf,
                         char **headers_out, size_t max_headerlen,
                         char **body_out, size_t *body_used, size_t max_bodylen,
diff --git a/src/or/rephist.c b/src/or/rephist.c
index b3fb2adcb..6f973be61 100644
--- a/src/or/rephist.c
+++ b/src/or/rephist.c
@@ -19,8 +19,9 @@ static void bw_arrays_init(void);
 static void predicted_ports_init(void);
 static void hs_usage_init(void);
 
-/**DOCDOC*/
+/** Total number of bytes currently allocated in fields used by rephist.c */
 uint64_t rephist_total_alloc=0;
+/** Number of or_history_t obects currently allocated */
 uint32_t rephist_total_num=0;
 
 /** If the total weighted run count of all runs for a router ever falls