Refactor begin cell parsing into its own function, with tests.

Add 'flags' argument to begin cells, per proposal 208.

src/or/connection_edge.c

@@ -8,6 +8,7 @@
  * \file connection_edge.c
  * \brief Handle edge streams.
  **/
+#define CONNECTION_EDGE_PRIVATE
 
 #include "or.h"
 #include "addressmap.h"
@@ -2059,6 +2060,64 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
   return;
 }
 
+/* DOCDOC */
+/* static */ int
+begin_cell_parse(const cell_t *cell, begin_cell_t *bcell,
+                 uint8_t *end_reason_out)
+{
+  relay_header_t rh;
+  const uint8_t *body, *nul;
+
+  memset(bcell, 0, sizeof(*bcell));
+  *end_reason_out = END_STREAM_REASON_MISC;
+
+  relay_header_unpack(&rh, cell->payload);
+  if (rh.length > RELAY_PAYLOAD_SIZE) {
+    return -2; /*XXXX why not TORPROTOL? */
+  }
+
+  bcell->stream_id = rh.stream_id;
+
+  if (rh.command == RELAY_COMMAND_BEGIN_DIR) {
+    bcell->is_begindir = 1;
+    return 0;
+  } else if (rh.command != RELAY_COMMAND_BEGIN) {
+    log_warn(LD_BUG, "Got an unexpected command %d", (int)rh.command);
+    *end_reason_out = END_STREAM_REASON_INTERNAL;
+    return -1;
+  }
+
+  body = cell->payload + RELAY_HEADER_SIZE;
+  nul = memchr(body, 0, rh.length);
+  if (! nul) {
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "Relay begin cell has no \\0. Closing.");
+    *end_reason_out = END_STREAM_REASON_TORPROTOCOL;
+    return -1;
+  }
+
+  if (tor_addr_port_split(LOG_PROTOCOL_WARN,
+                          (char*)(body),
+                          &bcell->address,&bcell->port)<0) {
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "Unable to parse addr:port in relay begin cell. Closing.");
+    *end_reason_out = END_STREAM_REASON_TORPROTOCOL;
+    return -1;
+  }
+  if (bcell->port == 0) {
+    log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
+           "Missing port in relay begin cell. Closing.");
+    tor_free(bcell->address);
+    *end_reason_out = END_STREAM_REASON_TORPROTOCOL;
+    return -1;
+  }
+  if (body + rh.length >= nul + 4)
+    bcell->flags = ntohl(get_uint32(nul+1));
+
+  return 0;
+}
+
+
 /** A relay 'begin' or 'begin_dir' cell has arrived, and either we are
  * an exit hop for the circuit, or we are the origin and it is a
  * rendezvous begin.
@@ -2082,10 +2141,13 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
 {
   edge_connection_t *n_stream;
   relay_header_t rh;
-  char *address=NULL;
-  uint16_t port;
+  char *address = NULL;
+  uint16_t port = 0;
   or_circuit_t *or_circ = NULL;
   const or_options_t *options = get_options();
+  begin_cell_t bcell;
+  int r;
+  uint8_t end_reason=0;
 
   assert_circuit_ok(circ);
   if (!CIRCUIT_IS_ORIGIN(circ))
@@ -2109,31 +2171,20 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
     return 0;
   }
 
-  if (rh.command == RELAY_COMMAND_BEGIN) {
-    if (!memchr(cell->payload+RELAY_HEADER_SIZE, 0, rh.length)) {
-      log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
-             "Relay begin cell has no \\0. Closing.");
-      relay_send_end_cell_from_edge(rh.stream_id, circ,
-                                    END_STREAM_REASON_TORPROTOCOL, NULL);
-      return 0;
-    }
-    if (tor_addr_port_split(LOG_PROTOCOL_WARN,
-                            (char*)(cell->payload+RELAY_HEADER_SIZE),
-                            &address,&port)<0) {
-      log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
-             "Unable to parse addr:port in relay begin cell. Closing.");
-      relay_send_end_cell_from_edge(rh.stream_id, circ,
-                                    END_STREAM_REASON_TORPROTOCOL, NULL);
-      return 0;
-    }
-    if (port==0) {
-      log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL,
-             "Missing port in relay begin cell. Closing.");
-      relay_send_end_cell_from_edge(rh.stream_id, circ,
-                                    END_STREAM_REASON_TORPROTOCOL, NULL);
-      tor_free(address);
-      return 0;
-    }
+  r = begin_cell_parse(cell, &bcell, &end_reason);
+  if (r < -1) {
+    return -1;
+  } else if (r == -1) {
+    tor_free(bcell.address);
+    relay_send_end_cell_from_edge(rh.stream_id, circ, end_reason, NULL);
+    return 0;
+  }
+
+  if (! bcell.is_begindir) {
+    /* Steal reference */
+    address = bcell.address;
+    port = bcell.port;
+
     if (or_circ && or_circ->p_chan) {
       if (!options->AllowSingleHopExits &&
            (or_circ->is_first_hop ||

src/or/connection_edge.h

@@ -91,5 +91,20 @@ int connection_edge_update_circuit_isolation(const entry_connection_t *conn,
                                              int dry_run);
 void circuit_clear_isolation(origin_circuit_t *circ);
 
+#ifdef CONNECTION_EDGE_PRIVATE
+/*DOCDOC*/
+typedef struct begin_cell_t {
+  char *address;
+  uint32_t flags;
+  uint16_t port;
+  uint16_t stream_id;
+  unsigned is_begindir : 1;
+} begin_cell_t;
+
+int begin_cell_parse(const cell_t *cell, begin_cell_t *bcell,
+                     uint8_t *end_reason_out);
+#endif
+
+
 #endif
 

src/test/include.am

@@ -14,6 +14,7 @@ src_test_AM_CPPFLAGS = -DSHARE_DATADIR="\"$(datadir)\"" \
 src_test_test_SOURCES = \
 	src/test/test.c \
 	src/test/test_addr.c \
+	src/test/test_cell_formats.c \
 	src/test/test_containers.c \
 	src/test/test_crypto.c \
 	src/test/test_data.c \

src/test/test.c

@@ -1983,6 +1983,7 @@ extern struct testcase_t pt_tests[];
 extern struct testcase_t config_tests[];
 extern struct testcase_t introduce_tests[];
 extern struct testcase_t replaycache_tests[];
+extern struct testcase_t cell_format_tests[];
 
 static struct testgroup_t testgroups[] = {
   { "", test_array },
@@ -1991,6 +1992,7 @@ static struct testgroup_t testgroups[] = {
   { "crypto/", crypto_tests },
   { "container/", container_tests },
   { "util/", util_tests },
+  { "cellfmt/", cell_format_tests },
   { "dir/", dir_tests },
   { "dir/md/", microdesc_tests },
   { "pt/", pt_tests },

src/test/test_cell_formats.c

+/* Copyright (c) 2001-2004, Roger Dingledine.
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2012, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+#include "orconfig.h"
+
+#define CONNECTION_EDGE_PRIVATE
+#include "or.h"
+#include "connection_edge.h"
+#include "relay.h"
+#include "test.h"
+
+#include <stdlib.h>
+#include <string.h>
+
+static void
+test_cfmt_relay_header(void *arg)
+{
+  relay_header_t rh;
+  const uint8_t hdr_1[RELAY_HEADER_SIZE] =
+    "\x03" "\x00\x00" "\x21\x22" "ABCD" "\x01\x03";
+  uint8_t hdr_out[RELAY_HEADER_SIZE];
+  (void)arg;
+
+  tt_int_op(sizeof(hdr_1), ==, RELAY_HEADER_SIZE);
+  relay_header_unpack(&rh, hdr_1);
+  tt_int_op(rh.command, ==, 3);
+  tt_int_op(rh.recognized, ==, 0);
+  tt_int_op(rh.stream_id, ==, 0x2122);
+  test_mem_op(rh.integrity, ==, "ABCD", 4);
+  tt_int_op(rh.length, ==, 0x103);
+
+  relay_header_pack(hdr_out, &rh);
+  test_mem_op(hdr_out, ==, hdr_1, RELAY_HEADER_SIZE);
+
+ done:
+  ;
+}
+
+static void
+make_relay_cell(cell_t *out, uint8_t command,
+                const void *body, size_t bodylen)
+{
+  relay_header_t rh;
+
+  memset(&rh, 0, sizeof(rh));
+  rh.stream_id = 5;
+  rh.command = command;
+  rh.length = bodylen;
+
+  out->command = CELL_RELAY;
+  out->circ_id = 10;
+  relay_header_pack(out->payload, &rh);
+
+  memcpy(out->payload + RELAY_HEADER_SIZE, body, bodylen);
+}
+
+static void
+test_cfmt_begin_cells(void *arg)
+{
+  cell_t cell;
+  begin_cell_t bcell;
+  uint8_t end_reason;
+  (void)arg;
+
+  /* Try begindir. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN_DIR, "", 0);
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_ptr_op(NULL, ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(0, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(1, ==, bcell.is_begindir);
+
+  /* A Begindir with extra stuff. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN_DIR, "12345", 5);
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_ptr_op(NULL, ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(0, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(1, ==, bcell.is_begindir);
+
+  /* A short but valid begin cell */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:9", 6);
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("a.b", ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(9, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* A significantly loner begin cell */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  {
+    const char c[] = "here-is-a-nice-long.hostname.com:65535";
+    make_relay_cell(&cell, RELAY_COMMAND_BEGIN, c, strlen(c)+1);
+  }
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("here-is-a-nice-long.hostname.com", ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(65535, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* An IPv4 begin cell. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "18.9.22.169:80", 15);
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("18.9.22.169", ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(80, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* An IPv6 begin cell. Let's make sure we handle colons*/
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN,
+                  "[2620::6b0:b:1a1a:0:26e5:480e]:80", 34);
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("[2620::6b0:b:1a1a:0:26e5:480e]", ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(80, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* a begin cell with extra junk but not enough for flags. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  {
+    const char c[] = "another.example.com:80\x00\x01\x02";
+    make_relay_cell(&cell, RELAY_COMMAND_BEGIN, c, sizeof(c)-1);
+  }
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("another.example.com", ==, bcell.address);
+  tt_int_op(0, ==, bcell.flags);
+  tt_int_op(80, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* a begin cell with flags. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  {
+    const char c[] = "another.example.com:443\x00\x01\x02\x03\x04";
+    make_relay_cell(&cell, RELAY_COMMAND_BEGIN, c, sizeof(c)-1);
+  }
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("another.example.com", ==, bcell.address);
+  tt_int_op(0x1020304, ==, bcell.flags);
+  tt_int_op(443, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* a begin cell with flags and even more cruft after that. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  {
+    const char c[] = "a-further.example.com:22\x00\xee\xaa\x00\xffHi mom";
+    make_relay_cell(&cell, RELAY_COMMAND_BEGIN, c, sizeof(c)-1);
+  }
+  tt_int_op(0, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  tt_str_op("a-further.example.com", ==, bcell.address);
+  tt_int_op(0xeeaa00ff, ==, bcell.flags);
+  tt_int_op(22, ==, bcell.port);
+  tt_int_op(5, ==, bcell.stream_id);
+  tt_int_op(0, ==, bcell.is_begindir);
+  tor_free(bcell.address);
+
+  /* bad begin cell: impossible length. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:80", 7);
+  cell.payload[9] = 0x01; /* Set length to 510 */
+  cell.payload[10] = 0xfe;
+  {
+    relay_header_t rh;
+    relay_header_unpack(&rh, cell.payload);
+    tt_int_op(rh.length, ==, 510);
+  }
+  tt_int_op(-2, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* Bad begin cell: no body. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "", 0);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* bad begin cell: no body. */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "", 0);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* bad begin cell: no colon */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b", 4);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* bad begin cell: no ports */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:", 5);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* bad begin cell: bad port */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:xyz", 8);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:100000", 11);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+  /* bad begin cell: no nul */
+  memset(&bcell, 0x7f, sizeof(bcell));
+  make_relay_cell(&cell, RELAY_COMMAND_BEGIN, "a.b:80", 6);
+  tt_int_op(-1, ==, begin_cell_parse(&cell, &bcell, &end_reason));
+
+ done:
+  tor_free(bcell.address);
+}
+
+#define TEST(name, flags)                                               \
+  { #name, test_cfmt_ ## name, flags, 0, NULL }
+
+struct testcase_t cell_format_tests[] = {
+  TEST(relay_header, 0),
+  TEST(begin_cells, 0),
+  END_OF_TESTCASES
+};
+