Ticket #24734: b24734-003.patch

File b24734-003.patch, 16.6 KB (added by neel, 20 months ago)

[PATCH] Remove the return value from the fascist_firewall_choose_address_* family of functions (Revision 3)

  • src/or/policies.c

    From 51702cb799aa68944778584a19450d45e8c66d14 Mon Sep 17 00:00:00 2001
    From: Neel Chauhan <neel@neelc.org>
    Date: Mon, 9 Apr 2018 21:12:33 -0400
    Subject: [PATCH 1/2] Initialize ap in the fascist_firewall_choose_address_*
     family of functions to 0
    
    ---
     src/or/policies.c | 18 ++++++++++++++++--
     1 file changed, 16 insertions(+), 2 deletions(-)
    
    diff --git a/src/or/policies.c b/src/or/policies.c
    index f718ded32..3d2fd93ef 100644
    a b fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, 
    845845  tor_assert(ipv6_addr);
    846846  tor_assert(ap);
    847847
     848  tor_addr_make_null(&ap->addr, AF_UNSPEC);
     849  ap->port = 0;
     850
    848851  tor_addr_port_t ipv4_ap;
    849852  tor_addr_copy(&ipv4_ap.addr, ipv4_addr);
    850853  ipv4_ap.port = (fw_connection == FIREWALL_OR_CONNECTION
    fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, 
    867870    ap->port = result->port;
    868871    return 1;
    869872  } else {
    870     tor_addr_make_null(&ap->addr, AF_UNSPEC);
    871     ap->port = 0;
    872873    return 0;
    873874  }
    874875}
    fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr, 
    889890{
    890891  tor_addr_t ipv4_addr;
    891892  tor_addr_from_ipv4h(&ipv4_addr, ipv4h_addr);
     893
     894  tor_addr_make_null(&ap->addr, AF_UNSPEC);
     895  ap->port = 0;
     896
    892897  return fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
    893898                                              ipv4_dirport, ipv6_addr,
    894899                                              ipv6_orport, ipv6_dirport,
    fascist_firewall_choose_address_rs(const routerstatus_t *rs, 
    955960
    956961  tor_assert(ap);
    957962
     963  tor_addr_make_null(&ap->addr, AF_UNSPEC);
     964  ap->port = 0;
     965
    958966  const or_options_t *options = get_options();
    959967  const node_t *node = node_get_by_id(rs->identity_digest);
    960968
    fascist_firewall_choose_address_node(const node_t *node, 
    9971005
    9981006  node_assert_ok(node);
    9991007
     1008  tor_addr_make_null(&ap->addr, AF_UNSPEC);
     1009  ap->port = 0;
     1010
    10001011  /* Calling fascist_firewall_choose_address_node() when the node is missing
    10011012   * IPv6 information breaks IPv6-only clients.
    10021013   * If the node is a hard-coded fallback directory or authority, call
    fascist_firewall_choose_address_dir_server(const dir_server_t *ds, 
    10471058    return 0;
    10481059  }
    10491060
     1061  tor_addr_make_null(&ap->addr, AF_UNSPEC);
     1062  ap->port = 0;
     1063
    10501064  /* A dir_server_t always has a fake_status. As long as it has the same
    10511065   * addresses/ports in both fake_status and dir_server_t, this works fine.
    10521066   * (See #17867.)
  • new file changes/ticket24734

    -- 
    2.17.0
    
    
    From 121d604c644dfe6d89874227f59746d143230046 Mon Sep 17 00:00:00 2001
    From: Neel Chauhan <neel@neelc.org>
    Date: Mon, 9 Apr 2018 21:43:11 -0400
    Subject: [PATCH 2/2] Remove the return value from the
     fascist_firewall_choose_address_* family of functions
    
    ---
     changes/ticket24734    |  6 ++++
     src/or/circuitbuild.c  |  9 ++----
     src/or/directory.c     | 12 ++++----
     src/or/policies.c      | 70 ++++++++++++++++--------------------------
     src/or/policies.h      | 14 ++++-----
     src/test/test_policy.c | 14 +++------
     6 files changed, 53 insertions(+), 72 deletions(-)
     create mode 100644 changes/ticket24734
    
    diff --git a/changes/ticket24734 b/changes/ticket24734
    new file mode 100644
    index 000000000..00029ce57
    - +  
     1  o Code simplification and refactoring:
     2    - Remove the return value for fascist_firewall_choose_address_base(),
     3      and sister functions such as fascist_firewall_choose_address_node()
     4      and fascist_firewall_choose_address_rs(). Also, while we're here,
     5      initialize the ap argument as leaving it uninitialized can pose a
     6      security hazard. Closes ticket 24734. Patch by Neel Chauhan.
  • src/or/circuitbuild.c

    diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
    index 488f6a214..e74ac9a43 100644
    a b extend_info_from_node(const node_t *node, int for_direct_connect) 
    28012801      return NULL;
    28022802  }
    28032803
    2804   /* Choose a preferred address first, but fall back to an allowed address.
    2805    * choose_address returns 1 on success, but get_prim_orport returns 0. */
     2804  /* Choose a preferred address first, but fall back to an allowed address. */
    28062805  if (for_direct_connect)
    2807     valid_addr = fascist_firewall_choose_address_node(node,
    2808                                                       FIREWALL_OR_CONNECTION,
    2809                                                       0, &ap);
     2806    fascist_firewall_choose_address_node(node, FIREWALL_OR_CONNECTION, 0, &ap);
    28102807  else {
    28112808    node_get_prim_orport(node, &ap);
    2812     valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
    28132809  }
     2810  valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
    28142811
    28152812  if (valid_addr)
    28162813    log_debug(LD_CIRC, "using %s for %s",
  • src/or/directory.c

    diff --git a/src/or/directory.c b/src/or/directory.c
    index c419b61d0..d6ce9290a 100644
    a b directory_choose_address_routerstatus(const routerstatus_t *status, 
    794794     * Use the preferred address and port if they are reachable, otherwise,
    795795     * use the alternate address and port (if any).
    796796     */
    797     have_or = fascist_firewall_choose_address_rs(status,
    798                                                  FIREWALL_OR_CONNECTION, 0,
    799                                                  use_or_ap);
     797    fascist_firewall_choose_address_rs(status, FIREWALL_OR_CONNECTION, 0,
     798                                       use_or_ap);
     799    have_or = tor_addr_port_is_valid_ap(use_or_ap, 0);
    800800  }
    801801
    802802  /* DirPort connections
    directory_choose_address_routerstatus(const routerstatus_t *status, 
    805805      indirection == DIRIND_ANON_DIRPORT ||
    806806      (indirection == DIRIND_ONEHOP
    807807       && !directory_must_use_begindir(options))) {
    808     have_dir = fascist_firewall_choose_address_rs(status,
    809                                                   FIREWALL_DIR_CONNECTION, 0,
    810                                                   use_dir_ap);
     808    fascist_firewall_choose_address_rs(status, FIREWALL_DIR_CONNECTION, 0,
     809                                       use_dir_ap);
     810    have_dir = tor_addr_port_is_valid_ap(use_dir_ap, 0);
    811811  }
    812812
    813813  /* We rejected all addresses in the relay's status. This means we can't
  • src/or/policies.c

    diff --git a/src/or/policies.c b/src/or/policies.c
    index 3d2fd93ef..6f16b3a32 100644
    a b fascist_firewall_choose_address(const tor_addr_port_t *a, 
    825825 * If pref_only, only choose preferred addresses. In either case, choose
    826826 * a preferred address before an address that's not preferred.
    827827 * If both addresses could be chosen (they are both preferred or both allowed)
    828  * choose IPv6 if pref_ipv6 is true, otherwise choose IPv4.
    829  * If neither address is chosen, return 0, else return 1. */
    830 static int
     828 * choose IPv6 if pref_ipv6 is true, otherwise choose IPv4. */
     829static void
    831830fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
    832831                                     uint16_t ipv4_orport,
    833832                                     uint16_t ipv4_dirport,
    fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, 
    868867  if (result) {
    869868    tor_addr_copy(&ap->addr, &result->addr);
    870869    ap->port = result->port;
    871     return 1;
    872   } else {
    873     return 0;
    874870  }
    875871}
    876872
    877873/** Like fascist_firewall_choose_address_base(), but takes a host-order IPv4
    878874 * address as the first parameter. */
    879 static int
     875static void
    880876fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr,
    881877                                      uint16_t ipv4_orport,
    882878                                      uint16_t ipv4_dirport,
    fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr, 
    894890  tor_addr_make_null(&ap->addr, AF_UNSPEC);
    895891  ap->port = 0;
    896892
    897   return fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
    898                                               ipv4_dirport, ipv6_addr,
    899                                               ipv6_orport, ipv6_dirport,
    900                                               fw_connection, pref_only,
    901                                               pref_ipv6, ap);
     893  fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
     894                                       ipv4_dirport, ipv6_addr,
     895                                       ipv6_orport, ipv6_dirport,
     896                                       fw_connection, pref_only,
     897                                       pref_ipv6, ap);
    902898}
    903899
    904900/* Some microdescriptor consensus methods have no IPv6 addresses in rs: they
    node_awaiting_ipv6(const or_options_t* options, const node_t *node) 
    949945 * This should only happen when there's no valid consensus, and rs doesn't
    950946 * correspond to a bridge client's bridge.
    951947 */
    952 int
     948void
    953949fascist_firewall_choose_address_rs(const routerstatus_t *rs,
    954950                                   firewall_connection_t fw_connection,
    955951                                   int pref_only, tor_addr_port_t* ap)
    956952{
    957953  if (!rs) {
    958     return 0;
     954    return;
    959955  }
    960956
    961957  tor_assert(ap);
    fascist_firewall_choose_address_rs(const routerstatus_t *rs, 
    967963  const node_t *node = node_get_by_id(rs->identity_digest);
    968964
    969965  if (node && !node_awaiting_ipv6(options, node)) {
    970     return fascist_firewall_choose_address_node(node, fw_connection, pref_only,
    971                                                 ap);
     966    fascist_firewall_choose_address_node(node, fw_connection, pref_only, ap);
    972967  } else {
    973968    /* There's no node-specific IPv6 preference, so use the generic IPv6
    974969     * preference instead. */
    fascist_firewall_choose_address_rs(const routerstatus_t *rs, 
    978973
    979974    /* Assume IPv4 and IPv6 DirPorts are the same.
    980975     * Assume the IPv6 OR and Dir addresses are the same. */
    981     return fascist_firewall_choose_address_ipv4h(rs->addr,
    982                                                  rs->or_port,
    983                                                  rs->dir_port,
    984                                                  &rs->ipv6_addr,
    985                                                  rs->ipv6_orport,
    986                                                  rs->dir_port,
    987                                                  fw_connection,
    988                                                  pref_only,
    989                                                  pref_ipv6,
    990                                                  ap);
     976    fascist_firewall_choose_address_ipv4h(rs->addr, rs->or_port, rs->dir_port,
     977                                          &rs->ipv6_addr, rs->ipv6_orport,
     978                                          rs->dir_port, fw_connection,
     979                                          pref_only, pref_ipv6, ap);
    991980  }
    992981}
    993982
    994983/** Like fascist_firewall_choose_address_base(), but takes <b>node</b>, and
    995984 * looks up the node's IPv6 preference rather than taking an argument
    996985 * for pref_ipv6. */
    997 int
     986void
    998987fascist_firewall_choose_address_node(const node_t *node,
    999988                                     firewall_connection_t fw_connection,
    1000989                                     int pref_only, tor_addr_port_t *ap)
    1001990{
    1002991  if (!node) {
    1003     return 0;
     992    return;
    1004993  }
    1005994
    1006995  node_assert_ok(node);
    fascist_firewall_choose_address_node(const node_t *node, 
    10171006   * descriptor (routerinfo), or is one of our configured bridges before
    10181007   * calling this function. */
    10191008  if (BUG(node_awaiting_ipv6(get_options(), node))) {
    1020     return 0;
     1009    return;
    10211010  }
    10221011
    10231012  const int pref_ipv6_node = (fw_connection == FIREWALL_OR_CONNECTION
    fascist_firewall_choose_address_node(const node_t *node, 
    10351024  node_get_pref_ipv6_dirport(node, &ipv6_dir_ap);
    10361025
    10371026  /* Assume the IPv6 OR and Dir addresses are the same. */
    1038   return fascist_firewall_choose_address_base(&ipv4_or_ap.addr,
    1039                                               ipv4_or_ap.port,
    1040                                               ipv4_dir_ap.port,
    1041                                               &ipv6_or_ap.addr,
    1042                                               ipv6_or_ap.port,
    1043                                               ipv6_dir_ap.port,
    1044                                               fw_connection,
    1045                                               pref_only,
    1046                                               pref_ipv6_node,
    1047                                               ap);
     1027  fascist_firewall_choose_address_base(&ipv4_or_ap.addr, ipv4_or_ap.port,
     1028                                       ipv4_dir_ap.port, &ipv6_or_ap.addr,
     1029                                       ipv6_or_ap.port, ipv6_dir_ap.port,
     1030                                       fw_connection, pref_only,
     1031                                       pref_ipv6_node, ap);
    10481032}
    10491033
    10501034/** Like fascist_firewall_choose_address_rs(), but takes <b>ds</b>. */
    1051 int
     1035void
    10521036fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
    10531037                                           firewall_connection_t fw_connection,
    10541038                                           int pref_only,
    10551039                                           tor_addr_port_t *ap)
    10561040{
    10571041  if (!ds) {
    1058     return 0;
     1042    return;
    10591043  }
    10601044
    10611045  tor_addr_make_null(&ap->addr, AF_UNSPEC);
    fascist_firewall_choose_address_dir_server(const dir_server_t *ds, 
    10661050   * (See #17867.)
    10671051   * This function relies on fascist_firewall_choose_address_rs looking up the
    10681052   * node if it can, because that will get the latest info for the relay. */
    1069   return fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
    1070                                             pref_only, ap);
     1053  fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
     1054                                     pref_only, ap);
    10711055}
    10721056
    10731057/** Return 1 if <b>addr</b> is permitted to connect to our dir port,
  • src/or/policies.h

    diff --git a/src/or/policies.h b/src/or/policies.h
    index 35220a812..4879acdd8 100644
    a b int fascist_firewall_allows_dir_server(const dir_server_t *ds, 
    5555                                       firewall_connection_t fw_connection,
    5656                                       int pref_only);
    5757
    58 int fascist_firewall_choose_address_rs(const routerstatus_t *rs,
    59                                        firewall_connection_t fw_connection,
    60                                        int pref_only, tor_addr_port_t* ap);
    61 int fascist_firewall_choose_address_node(const node_t *node,
    62                                          firewall_connection_t fw_connection,
    63                                          int pref_only, tor_addr_port_t* ap);
    64 int fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
     58void fascist_firewall_choose_address_rs(const routerstatus_t *rs,
     59                                        firewall_connection_t fw_connection,
     60                                        int pref_only, tor_addr_port_t* ap);
     61void fascist_firewall_choose_address_node(const node_t *node,
     62                                          firewall_connection_t fw_connection,
     63                                          int pref_only, tor_addr_port_t* ap);
     64void fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
    6565                                          firewall_connection_t fw_connection,
    6666                                          int pref_only, tor_addr_port_t* ap);
    6767
  • src/test/test_policy.c

    diff --git a/src/test/test_policy.c b/src/test/test_policy.c
    index f8aa8ac40..f18058586 100644
    a b test_policies_fascist_firewall_allows_address(void *arg) 
    19231923    tor_addr_port_t chosen_rs_ap; \
    19241924    tor_addr_make_null(&chosen_rs_ap.addr, AF_INET); \
    19251925    chosen_rs_ap.port = 0; \
    1926     tt_int_op(fascist_firewall_choose_address_rs(&(fake_rs), \
    1927                                                  (fw_connection), \
    1928                                                  (pref_only), \
    1929                                                  &chosen_rs_ap), \
    1930               OP_EQ, (expect_rv)); \
     1926    fascist_firewall_choose_address_rs(&(fake_rs), (fw_connection), \
     1927                                       (pref_only), &chosen_rs_ap); \
    19311928    tt_assert(tor_addr_eq(&(expect_ap).addr, &chosen_rs_ap.addr)); \
    19321929    tt_int_op((expect_ap).port, OP_EQ, chosen_rs_ap.port); \
    19331930  STMT_END
    test_policies_fascist_firewall_allows_address(void *arg) 
    19401937    tor_addr_port_t chosen_node_ap; \
    19411938    tor_addr_make_null(&chosen_node_ap.addr, AF_INET); \
    19421939    chosen_node_ap.port = 0; \
    1943     tt_int_op(fascist_firewall_choose_address_node(&(fake_node), \
    1944                                                    (fw_connection), \
    1945                                                    (pref_only), \
    1946                                                    &chosen_node_ap), \
    1947               OP_EQ, (expect_rv)); \
     1940    fascist_firewall_choose_address_node(&(fake_node),(fw_connection), \
     1941                                         (pref_only), &chosen_node_ap); \
    19481942    tt_assert(tor_addr_eq(&(expect_ap).addr, &chosen_node_ap.addr)); \
    19491943    tt_int_op((expect_ap).port, OP_EQ, chosen_node_ap.port); \
    19501944  STMT_END