Ticket #24734: b24734-002.patch

File b24734-002.patch, 16.0 KB (added by neel, 20 months ago)

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

  • src/or/policies.c

    From 84830cd9f1aab05a71eea214df1df01995f763e8 Mon Sep 17 00:00:00 2001
    From: Neel Chauhan <neel@neelc.org>
    Date: Sun, 8 Apr 2018 12:40:53 -0400
    Subject: [PATCH 1/2] Initialize ap in the fascist_firewall_choose_address_*
     family of functions to 0
    
    ---
     src/or/policies.c | 11 +++++++++++
     1 file changed, 11 insertions(+)
    
    diff --git a/src/or/policies.c b/src/or/policies.c
    index f718ded32..8a75f39d0 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  memset(ap, 0, sizeof(tor_addr_port_t));
     849
    848850  tor_addr_port_t ipv4_ap;
    849851  tor_addr_copy(&ipv4_ap.addr, ipv4_addr);
    850852  ipv4_ap.port = (fw_connection == FIREWALL_OR_CONNECTION
    fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr, 
    889891{
    890892  tor_addr_t ipv4_addr;
    891893  tor_addr_from_ipv4h(&ipv4_addr, ipv4h_addr);
     894
     895  memset(ap, 0, sizeof(tor_addr_port_t));
     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  memset(ap, 0, sizeof(tor_addr_port_t));
     964
    958965  const or_options_t *options = get_options();
    959966  const node_t *node = node_get_by_id(rs->identity_digest);
    960967
    fascist_firewall_choose_address_node(const node_t *node, 
    10091016    return 0;
    10101017  }
    10111018
     1019  memset(ap, 0, sizeof(tor_addr_port_t));
     1020
    10121021  const int pref_ipv6_node = (fw_connection == FIREWALL_OR_CONNECTION
    10131022                              ? node_ipv6_or_preferred(node)
    10141023                              : node_ipv6_dir_preferred(node));
    fascist_firewall_choose_address_dir_server(const dir_server_t *ds, 
    10471056    return 0;
    10481057  }
    10491058
     1059  memset(ap, 0, sizeof(tor_addr_port_t));
     1060
    10501061  /* A dir_server_t always has a fake_status. As long as it has the same
    10511062   * addresses/ports in both fake_status and dir_server_t, this works fine.
    10521063   * (See #17867.)
  • new file changes/ticket24734

    -- 
    2.17.0
    
    
    From 0e6adaadc13852639e9b22192cce9300d69eaa29 Mon Sep 17 00:00:00 2001
    From: Neel Chauhan <neel@neelc.org>
    Date: Sun, 8 Apr 2018 13:32:55 -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  |  6 ++--
     src/or/directory.c     | 12 ++++----
     src/or/policies.c      | 66 +++++++++++++++++-------------------------
     src/or/policies.h      | 14 ++++-----
     src/test/test_policy.c | 14 +++------
     6 files changed, 51 insertions(+), 67 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..865d0a5e0 100644
    a b extend_info_from_node(const node_t *node, int for_direct_connect) 
    28042804  /* Choose a preferred address first, but fall back to an allowed address.
    28052805   * choose_address returns 1 on success, but get_prim_orport returns 0. */
    28062806  if (for_direct_connect)
    2807     valid_addr = fascist_firewall_choose_address_node(node,
    2808                                                       FIREWALL_OR_CONNECTION,
    2809                                                       0, &ap);
     2807    fascist_firewall_choose_address_node(node, FIREWALL_OR_CONNECTION, 0, &ap);
    28102808  else {
    28112809    node_get_prim_orport(node, &ap);
    2812     valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
    28132810  }
     2811  valid_addr = tor_addr_port_is_valid_ap(&ap, 0);
    28142812
    28152813  if (valid_addr)
    28162814    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 8a75f39d0..d4a9993e3 100644
    a b fascist_firewall_choose_address(const tor_addr_port_t *a, 
    827827 * If both addresses could be chosen (they are both preferred or both allowed)
    828828 * choose IPv6 if pref_ipv6 is true, otherwise choose IPv4.
    829829 * If neither address is chosen, return 0, else return 1. */
    830 static int
     830static void
    831831fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr,
    832832                                     uint16_t ipv4_orport,
    833833                                     uint16_t ipv4_dirport,
    fascist_firewall_choose_address_base(const tor_addr_t *ipv4_addr, 
    867867  if (result) {
    868868    tor_addr_copy(&ap->addr, &result->addr);
    869869    ap->port = result->port;
    870     return 1;
    871870  } else {
    872871    tor_addr_make_null(&ap->addr, AF_UNSPEC);
    873872    ap->port = 0;
    874     return 0;
    875873  }
    876874}
    877875
    878876/** Like fascist_firewall_choose_address_base(), but takes a host-order IPv4
    879877 * address as the first parameter. */
    880 static int
     878static void
    881879fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr,
    882880                                      uint16_t ipv4_orport,
    883881                                      uint16_t ipv4_dirport,
    fascist_firewall_choose_address_ipv4h(uint32_t ipv4h_addr, 
    894892
    895893  memset(ap, 0, sizeof(tor_addr_port_t));
    896894
    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);
     895  fascist_firewall_choose_address_base(&ipv4_addr, ipv4_orport,
     896                                       ipv4_dirport, ipv6_addr,
     897                                       ipv6_orport, ipv6_dirport,
     898                                       fw_connection, pref_only,
     899                                       pref_ipv6, ap);
    902900}
    903901
    904902/* Some microdescriptor consensus methods have no IPv6 addresses in rs: they
    node_awaiting_ipv6(const or_options_t* options, const node_t *node) 
    949947 * This should only happen when there's no valid consensus, and rs doesn't
    950948 * correspond to a bridge client's bridge.
    951949 */
    952 int
     950void
    953951fascist_firewall_choose_address_rs(const routerstatus_t *rs,
    954952                                   firewall_connection_t fw_connection,
    955953                                   int pref_only, tor_addr_port_t* ap)
    956954{
    957955  if (!rs) {
    958     return 0;
     956    return;
    959957  }
    960958
    961959  tor_assert(ap);
    fascist_firewall_choose_address_rs(const routerstatus_t *rs, 
    966964  const node_t *node = node_get_by_id(rs->identity_digest);
    967965
    968966  if (node && !node_awaiting_ipv6(options, node)) {
    969     return fascist_firewall_choose_address_node(node, fw_connection, pref_only,
    970                                                 ap);
     967    fascist_firewall_choose_address_node(node, fw_connection, pref_only, ap);
    971968  } else {
    972969    /* There's no node-specific IPv6 preference, so use the generic IPv6
    973970     * preference instead. */
    fascist_firewall_choose_address_rs(const routerstatus_t *rs, 
    977974
    978975    /* Assume IPv4 and IPv6 DirPorts are the same.
    979976     * Assume the IPv6 OR and Dir addresses are the same. */
    980     return fascist_firewall_choose_address_ipv4h(rs->addr,
    981                                                  rs->or_port,
    982                                                  rs->dir_port,
    983                                                  &rs->ipv6_addr,
    984                                                  rs->ipv6_orport,
    985                                                  rs->dir_port,
    986                                                  fw_connection,
    987                                                  pref_only,
    988                                                  pref_ipv6,
    989                                                  ap);
     977    fascist_firewall_choose_address_ipv4h(rs->addr, rs->or_port, rs->dir_port,
     978                                          &rs->ipv6_addr, rs->ipv6_orport,
     979                                          rs->dir_port, fw_connection,
     980                                          pref_only, pref_ipv6, ap);
    990981  }
    991982}
    992983
    993984/** Like fascist_firewall_choose_address_base(), but takes <b>node</b>, and
    994985 * looks up the node's IPv6 preference rather than taking an argument
    995986 * for pref_ipv6. */
    996 int
     987void
    997988fascist_firewall_choose_address_node(const node_t *node,
    998989                                     firewall_connection_t fw_connection,
    999990                                     int pref_only, tor_addr_port_t *ap)
    1000991{
    1001992  if (!node) {
    1002     return 0;
     993    return;
    1003994  }
    1004995
    1005996  node_assert_ok(node);
    fascist_firewall_choose_address_node(const node_t *node, 
    10131004   * descriptor (routerinfo), or is one of our configured bridges before
    10141005   * calling this function. */
    10151006  if (BUG(node_awaiting_ipv6(get_options(), node))) {
    1016     return 0;
     1007    return;
    10171008  }
    10181009
    10191010  memset(ap, 0, sizeof(tor_addr_port_t));
    fascist_firewall_choose_address_node(const node_t *node, 
    10331024  node_get_pref_ipv6_dirport(node, &ipv6_dir_ap);
    10341025
    10351026  /* Assume the IPv6 OR and Dir addresses are the same. */
    1036   return fascist_firewall_choose_address_base(&ipv4_or_ap.addr,
    1037                                               ipv4_or_ap.port,
    1038                                               ipv4_dir_ap.port,
    1039                                               &ipv6_or_ap.addr,
    1040                                               ipv6_or_ap.port,
    1041                                               ipv6_dir_ap.port,
    1042                                               fw_connection,
    1043                                               pref_only,
    1044                                               pref_ipv6_node,
    1045                                               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);
    10461032}
    10471033
    10481034/** Like fascist_firewall_choose_address_rs(), but takes <b>ds</b>. */
    1049 int
     1035void
    10501036fascist_firewall_choose_address_dir_server(const dir_server_t *ds,
    10511037                                           firewall_connection_t fw_connection,
    10521038                                           int pref_only,
    10531039                                           tor_addr_port_t *ap)
    10541040{
    10551041  if (!ds) {
    1056     return 0;
     1042    return;
    10571043  }
    10581044
    10591045  memset(ap, 0, sizeof(tor_addr_port_t));
    fascist_firewall_choose_address_dir_server(const dir_server_t *ds, 
    10631049   * (See #17867.)
    10641050   * This function relies on fascist_firewall_choose_address_rs looking up the
    10651051   * node if it can, because that will get the latest info for the relay. */
    1066   return fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
    1067                                             pref_only, ap);
     1052  fascist_firewall_choose_address_rs(&ds->fake_status, fw_connection,
     1053                                     pref_only, ap);
    10681054}
    10691055
    10701056/** 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..924ca356f 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