diff options
49 files changed, 1726 insertions, 803 deletions
diff --git a/Makefile.am b/Makefile.am index b40b2e51bc..415c2f2b4e 100644 --- a/Makefile.am +++ b/Makefile.am @@ -161,7 +161,9 @@ EXTRA_DIST+= \ README \ ReleaseNotes \ scripts/maint/checkIncludes.py \ - scripts/maint/checkSpace.pl + scripts/maint/checkSpace.pl \ + scripts/maint/practracker + ## This tells etags how to find mockable function definitions. AM_ETAGSFLAGS=--regex='{c}/MOCK_IMPL([^,]+,\W*\([a-zA-Z0-9_]+\)\W*,/\1/s' @@ -224,7 +226,7 @@ shellcheck: fi; \ fi -check-local: check-spaces check-changes check-includes shellcheck +check-local: check-spaces check-changes check-includes check-best-practices shellcheck need-chutney-path: @if test ! -d "$$CHUTNEY_PATH"; then \ @@ -345,6 +347,11 @@ if USEPYTHON $(PYTHON) $(top_srcdir)/scripts/maint/checkIncludes.py endif +check-best-practices: +if USEPYTHON + $(PYTHON) $(top_srcdir)/scripts/maint/practracker/practracker.py $(top_srcdir) +endif + check-docs: all $(PERL) $(top_builddir)/scripts/maint/checkOptionDocs.pl diff --git a/changes/bug22781 b/changes/bug22781 new file mode 100644 index 0000000000..5606dfa5e2 --- /dev/null +++ b/changes/bug22781 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Replace hs_desc_link_specifier_t with link_specifier_t, + and remove all hs_desc_link_specifier_t-specific code. + Fixes bug 22781; bugfix on 0.3.2.1-alpha. diff --git a/changes/bug23576 b/changes/bug23576 new file mode 100644 index 0000000000..edcae02e5e --- /dev/null +++ b/changes/bug23576 @@ -0,0 +1,7 @@ + o Minor features (IPv6, v3 onion services): + - Make v3 onion services put IPv6 addresses in service + descriptors. Before this change, service descriptors only + contained IPv4 addressesd. Implements 26992. + o Code simplification and refactoring: + - Simplify v3 onion service link specifier handling code. + Fixes bug 23576; bugfix on 0.3.2.1-alpha. diff --git a/changes/bug28525 b/changes/bug28525 new file mode 100644 index 0000000000..988ffb2192 --- /dev/null +++ b/changes/bug28525 @@ -0,0 +1,7 @@ + o Minor features (address selection): + - Make Tor aware of the RFC 6598 (Carrier Grade NAT) IP range, which is the + subnet 100.64.0.0/10. This is deployed by many ISPs as an alternative to + RFC 1918 that does not break existing internal networks. This patch fixes + security issues caused by RFC 6518 by blocking control ports on these + addresses and warns users if client ports or ExtORPorts are listening on + a RFC 6598 address. Closes ticket 28525. Patch by Neel Chauhan. diff --git a/changes/bug29221 b/changes/bug29221 new file mode 100644 index 0000000000..fbe08aa9a0 --- /dev/null +++ b/changes/bug29221 @@ -0,0 +1,5 @@ + o Minor features (development tools): + - Tor's test scripts now check for files and functions that seem + too long and complicated. Existing overlong functions and files are + accepted for now, but should eventually be refactored. Closes + ticket 29221. diff --git a/changes/bug29243 b/changes/bug29243 new file mode 100644 index 0000000000..b5694f7568 --- /dev/null +++ b/changes/bug29243 @@ -0,0 +1,3 @@ + o Minor bugfixes (testing, v3 onion services): + - Fix some incorrect code in the v3 onion service unit tests. + Fixes bug 29243; bugfix on 0.3.2.1-alpha. diff --git a/changes/bug29298 b/changes/bug29298 new file mode 100644 index 0000000000..6e447b62dd --- /dev/null +++ b/changes/bug29298 @@ -0,0 +1,6 @@ + o Minor features (circuit padding): + - Allow the padding machine designer to pick the edges of their histogram + instead of trying to compute them automatically using an exponential + formula. Resolves some undefined behavior in the case of small histograms + and allows greater flexibility on machine design. Closes ticket 29298; + bugfix on 0.4.0.1-alpha.
\ No newline at end of file diff --git a/changes/bug29706_minimal b/changes/bug29706_minimal new file mode 100644 index 0000000000..9d4a43326c --- /dev/null +++ b/changes/bug29706_minimal @@ -0,0 +1,4 @@ + o Minor bugfixes (memory management, testing): + - Stop leaking parts of the shared random state in the shared-random unit + tests. The previous fix in 29599 was incomplete. + Fixes bug 29706; bugfix on 0.2.9.1-alpha. diff --git a/changes/bug29706_refactor b/changes/bug29706_refactor new file mode 100644 index 0000000000..ba1d0c7edd --- /dev/null +++ b/changes/bug29706_refactor @@ -0,0 +1,4 @@ + o Minor bugfixes (memory management): + - Refactor the shared random state's memory management so that it actually + takes ownership of the shared random value pointers. + Fixes bug 29706; bugfix on 0.2.9.1-alpha. diff --git a/doc/HACKING/HelpfulTools.md b/doc/HACKING/HelpfulTools.md index d499238526..cba57e875d 100644 --- a/doc/HACKING/HelpfulTools.md +++ b/doc/HACKING/HelpfulTools.md @@ -371,3 +371,18 @@ source code. Here's how to use it: 6. See the Doxygen manual for more information; this summary just scratches the surface. + +Style and best-pratices checking +-------------------------------- + +We use scripts to check for various problems in the formatting and style +of our source code. The "check-spaces" test detects a bunch of violations +of our coding style on the local level. The "check-best-practices" test +looks for violations of some of our complexity guidelines. + +You can tell the tool about exceptions to the complexity guidelines via its +exceptions file (scripts/maint/practracker/exceptions.txt). But before you +do this, consider whether you shouldn't fix the underlying problem. Maybe +that file really _is_ too big. Maybe that function really _is_ doing too +much. (On the other hand, for stable release series, it is sometimes better +to leave things unrefactored.) diff --git a/scripts/git/git-merge-forward.sh b/scripts/git/git-merge-forward.sh index 4b294f4945..67af7e98bf 100755 --- a/scripts/git/git-merge-forward.sh +++ b/scripts/git/git-merge-forward.sh @@ -53,6 +53,18 @@ RELEASE_040=( "release-0.4.0" "maint-0.4.0" "$GIT_PATH/$TOR_WKT_NAME/release-0.4 # from that repository. ORIGIN_PATH="$GIT_PATH/$TOR_MASTER_NAME" +# SC2034 -- shellcheck thinks that these are unused. We know better. +ACTUALLY_THESE_ARE_USED=<<EOF +${MAINT_034[0]} +${MAINT_035[0]} +${MAINT_040[0]} +${MAINT_MASTER[0]} +${RELEASE_029[0]} +${RELEASE_034[0]} +${RELEASE_035[0]} +${RELEASE_040[0]} +EOF + ########################## # Git Worktree to manage # ########################## diff --git a/scripts/git/git-pull-all.sh b/scripts/git/git-pull-all.sh index 9c6a0a8f45..0a4898a111 100755 --- a/scripts/git/git-pull-all.sh +++ b/scripts/git/git-pull-all.sh @@ -52,6 +52,19 @@ RELEASE_040=( "release-0.4.0" "$GIT_PATH/$TOR_WKT_NAME/release-0.4.0" ) # from that repository. ORIGIN_PATH="$GIT_PATH/$TOR_MASTER_NAME" +# SC2034 -- shellcheck thinks that these are unused. We know better. +ACTUALLY_THESE_ARE_USED=<<EOF +${MAINT_029[0]} +${MAINT_034[0]} +${MAINT_035[0]} +${MAINT_040[0]} +${MAINT_MASTER[0]} +${RELEASE_029[0]} +${RELEASE_034[0]} +${RELEASE_035[0]} +${RELEASE_040[0]} +EOF + ########################## # Git Worktree to manage # ########################## diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt new file mode 100644 index 0000000000..abe615f04a --- /dev/null +++ b/scripts/maint/practracker/exceptions.txt @@ -0,0 +1,261 @@ +problem function-size /src/core/proto/proto_socks.c:parse_socks_client() 112 +problem file-size /src/core/or/channel.c 3476 +problem function-size /src/core/or/scheduler_kist.c:kist_scheduler_run() 171 +problem function-size /src/core/or/scheduler_vanilla.c:vanilla_scheduler_run() 109 +problem function-size /src/core/or/command.c:command_process_create_cell() 156 +problem function-size /src/core/or/command.c:command_process_relay_cell() 132 +problem function-size /src/core/or/protover.c:protover_all_supported() 116 +problem function-size /src/core/or/circuitmux.c:circuitmux_detach_all_circuits() 109 +problem function-size /src/core/or/circuitmux.c:circuitmux_set_policy() 110 +problem function-size /src/core/or/circuitmux.c:circuitmux_attach_circuit() 129 +problem include-count /src/core/or/circuitlist.c 54 +problem function-size /src/core/or/circuitlist.c:HT_PROTOTYPE() 128 +problem function-size /src/core/or/circuitlist.c:circuit_free_() 137 +problem function-size /src/core/or/circuitlist.c:circuit_find_to_cannibalize() 102 +problem function-size /src/core/or/circuitlist.c:circuit_about_to_free() 120 +problem function-size /src/core/or/circuitlist.c:circuits_handle_oom() 117 +problem file-size /src/core/or/connection_or.c 3111 +problem include-count /src/core/or/connection_or.c 51 +problem function-size /src/core/or/connection_or.c:connection_or_group_set_badness_() 105 +problem function-size /src/core/or/connection_or.c:connection_or_client_learned_peer_id() 144 +problem function-size /src/core/or/connection_or.c:connection_or_compute_authenticate_cell_body() 235 +problem file-size /src/core/or/policies.c 3163 +problem function-size /src/core/or/policies.c:policy_summarize() 107 +problem function-size /src/core/or/channeltls.c:channel_tls_handle_var_cell() 160 +problem function-size /src/core/or/channeltls.c:channel_tls_process_versions_cell() 170 +problem function-size /src/core/or/channeltls.c:channel_tls_process_netinfo_cell() 214 +problem function-size /src/core/or/channeltls.c:channel_tls_process_certs_cell() 246 +problem function-size /src/core/or/channeltls.c:channel_tls_process_authenticate_cell() 202 +problem file-size /src/core/or/circuituse.c 3146 +problem function-size /src/core/or/circuituse.c:circuit_is_acceptable() 129 +problem function-size /src/core/or/circuituse.c:circuit_expire_building() 394 +problem function-size /src/core/or/circuituse.c:circuit_log_ancient_one_hop_circuits() 126 +problem function-size /src/core/or/circuituse.c:circuit_build_failed() 149 +problem function-size /src/core/or/circuituse.c:circuit_launch_by_extend_info() 110 +problem function-size /src/core/or/circuituse.c:circuit_get_open_circ_or_launch() 354 +problem function-size /src/core/or/circuituse.c:connection_ap_handshake_attach_circuit() 244 +problem file-size /src/core/or/connection_edge.c 4550 +problem include-count /src/core/or/connection_edge.c 64 +problem function-size /src/core/or/connection_edge.c:connection_ap_expire_beginning() 117 +problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_rewrite() 192 +problem function-size /src/core/or/connection_edge.c:connection_ap_handle_onion() 188 +problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_rewrite_and_attach() 423 +problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_send_begin() 111 +problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_socks_resolved() 106 +problem function-size /src/core/or/connection_edge.c:connection_exit_begin_conn() 184 +problem function-size /src/core/or/connection_edge.c:connection_exit_connect() 102 +problem function-size /src/core/or/circuitstats.c:circuit_build_times_parse_state() 124 +problem function-size /src/core/or/versions.c:tor_version_parse() 104 +problem file-size /src/core/or/circuitbuild.c 3060 +problem include-count /src/core/or/circuitbuild.c 53 +problem function-size /src/core/or/circuitbuild.c:get_unique_circ_id_by_chan() 128 +problem function-size /src/core/or/circuitbuild.c:circuit_extend() 147 +problem function-size /src/core/or/circuitbuild.c:choose_good_exit_server_general() 206 +problem file-size /src/core/or/relay.c 3183 +problem function-size /src/core/or/relay.c:circuit_receive_relay_cell() 123 +problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 101 +problem function-size /src/core/or/relay.c:connection_ap_process_end_not_open() 194 +problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell_not_open() 139 +problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell() 520 +problem function-size /src/core/or/relay.c:connection_edge_package_raw_inbuf() 130 +problem function-size /src/core/or/relay.c:circuit_resume_edge_reading_helper() 148 +problem file-size /src/core/mainloop/mainloop.c 3050 +problem include-count /src/core/mainloop/mainloop.c 65 +problem function-size /src/core/mainloop/mainloop.c:conn_close_if_marked() 108 +problem function-size /src/core/mainloop/mainloop.c:run_connection_housekeeping() 123 +problem function-size /src/core/mainloop/mainloop.c:CALLBACK() 116 +problem file-size /src/core/mainloop/connection.c 5547 +problem include-count /src/core/mainloop/connection.c 60 +problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 184 +problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328 +problem function-size /src/core/mainloop/connection.c:connection_handle_listener_read() 161 +problem function-size /src/core/mainloop/connection.c:connection_connect_sockaddr() 103 +problem function-size /src/core/mainloop/connection.c:connection_proxy_connect() 148 +problem function-size /src/core/mainloop/connection.c:connection_read_proxy_handshake() 153 +problem function-size /src/core/mainloop/connection.c:retry_listener_ports() 116 +problem function-size /src/core/mainloop/connection.c:connection_handle_read_impl() 111 +problem function-size /src/core/mainloop/connection.c:connection_buf_read_from_socket() 177 +problem function-size /src/core/mainloop/connection.c:connection_handle_write_impl() 241 +problem function-size /src/core/mainloop/connection.c:assert_connection_ok() 143 +problem function-size /src/app/config/confparse.c:config_assign_value() 205 +problem function-size /src/app/config/confparse.c:config_get_assigned_option() 129 +problem file-size /src/app/config/config.c 8488 +problem include-count /src/app/config/config.c 84 +problem function-size /src/app/config/config.c:options_act_reversible() 296 +problem function-size /src/app/config/config.c:options_act() 588 +problem function-size /src/app/config/config.c:resolve_my_address() 192 +problem function-size /src/app/config/config.c:options_validate() 1207 +problem function-size /src/app/config/config.c:options_init_from_torrc() 202 +problem function-size /src/app/config/config.c:options_init_from_string() 173 +problem function-size /src/app/config/config.c:options_init_logs() 146 +problem function-size /src/app/config/config.c:parse_bridge_line() 104 +problem function-size /src/app/config/config.c:parse_transport_line() 191 +problem function-size /src/app/config/config.c:parse_dir_authority_line() 151 +problem function-size /src/app/config/config.c:parse_dir_fallback_line() 102 +problem function-size /src/app/config/config.c:parse_port_config() 452 +problem function-size /src/app/config/config.c:parse_ports() 170 +problem function-size /src/app/config/config.c:getinfo_helper_config() 116 +problem function-size /src/app/main/ntmain.c:nt_service_install() 125 +problem include-count /src/app/main/main.c 83 +problem function-size /src/app/main/main.c:dumpstats() 102 +problem function-size /src/app/main/main.c:tor_init() 136 +problem function-size /src/app/main/main.c:sandbox_init_filter() 291 +problem function-size /src/app/main/main.c:run_tor_main_loop() 105 +problem function-size /src/tools/tor-resolve.c:build_socks5_resolve_request() 104 +problem function-size /src/tools/tor-resolve.c:do_resolve() 173 +problem function-size /src/tools/tor-resolve.c:main() 112 +problem function-size /src/tools/tor-gencert.c:parse_commandline() 111 +problem function-size /src/feature/keymgt/loadkey.c:ed_key_init_from_file() 333 +problem function-size /src/feature/dircommon/consdiff.c:gen_ed_diff() 204 +problem function-size /src/feature/dircommon/consdiff.c:apply_ed_diff() 159 +problem file-size /src/feature/control/control.c 7592 +problem include-count /src/feature/control/control.c 83 +problem function-size /src/feature/control/control.c:handle_control_authenticate() 188 +problem function-size /src/feature/control/control.c:getinfo_helper_misc() 109 +problem function-size /src/feature/control/control.c:getinfo_helper_dir() 304 +problem function-size /src/feature/control/control.c:getinfo_helper_events() 236 +problem function-size /src/feature/control/control.c:handle_control_extendcircuit() 151 +problem function-size /src/feature/control/control.c:handle_control_authchallenge() 115 +problem function-size /src/feature/control/control.c:handle_control_hsfetch() 114 +problem function-size /src/feature/control/control.c:handle_control_hspost() 117 +problem function-size /src/feature/control/control.c:handle_control_add_onion() 293 +problem function-size /src/feature/control/control.c:add_onion_helper_keyarg() 125 +problem function-size /src/feature/control/control.c:connection_control_process_inbuf() 239 +problem function-size /src/feature/control/control.c:control_event_stream_status() 119 +problem function-size /src/feature/stats/rephist.c:rep_hist_load_mtbf_data() 185 +problem function-size /src/feature/stats/rephist.c:rep_hist_format_exit_stats() 148 +problem function-size /src/feature/dircache/consdiffmgr.c:consdiffmgr_cleanup() 115 +problem function-size /src/feature/dircache/consdiffmgr.c:consdiffmgr_rescan_flavor_() 111 +problem function-size /src/feature/dircache/consdiffmgr.c:consensus_diff_worker_threadfn() 132 +problem function-size /src/feature/dircache/dircache.c:handle_get_current_consensus() 166 +problem function-size /src/feature/dircache/dircache.c:directory_handle_command_post() 120 +problem function-size /src/feature/hibernate/hibernate.c:accounting_parse_options() 109 +problem function-size /src/feature/relay/routerkeys.c:load_ed_keys() 294 +problem file-size /src/feature/relay/router.c 3221 +problem include-count /src/feature/relay/router.c 56 +problem function-size /src/feature/relay/router.c:init_keys() 252 +problem function-size /src/feature/relay/router.c:get_my_declared_family() 114 +problem function-size /src/feature/relay/router.c:router_build_fresh_descriptor() 190 +problem function-size /src/feature/relay/router.c:router_dump_router_to_string() 375 +problem function-size /src/feature/relay/router.c:extrainfo_dump_to_string() 208 +problem function-size /src/feature/relay/dns.c:dns_resolve_impl() 134 +problem function-size /src/feature/relay/dns.c:configure_nameservers() 161 +problem function-size /src/feature/relay/dns.c:evdns_callback() 109 +problem function-size /src/feature/dirparse/authcert_parse.c:authority_cert_parse_from_string() 182 +problem function-size /src/feature/dirparse/ns_parse.c:routerstatus_parse_entry_from_string() 286 +problem function-size /src/feature/dirparse/ns_parse.c:networkstatus_verify_bw_weights() 389 +problem function-size /src/feature/dirparse/ns_parse.c:networkstatus_parse_vote_from_string() 638 +problem function-size /src/feature/dirparse/parsecommon.c:tokenize_string() 103 +problem function-size /src/feature/dirparse/parsecommon.c:get_next_token() 159 +problem function-size /src/feature/dirparse/routerparse.c:router_parse_entry_from_string() 554 +problem function-size /src/feature/dirparse/routerparse.c:extrainfo_parse_entry_from_string() 210 +problem function-size /src/feature/dirparse/microdesc_parse.c:microdescs_parse_from_string() 154 +problem file-size /src/feature/dirauth/dirvote.c 4729 +problem include-count /src/feature/dirauth/dirvote.c 53 +problem function-size /src/feature/dirauth/dirvote.c:format_networkstatus_vote() 251 +problem function-size /src/feature/dirauth/dirvote.c:networkstatus_compute_bw_weights_v10() 235 +problem function-size /src/feature/dirauth/dirvote.c:networkstatus_compute_consensus() 962 +problem function-size /src/feature/dirauth/dirvote.c:networkstatus_add_detached_signatures() 123 +problem function-size /src/feature/dirauth/dirvote.c:dirvote_add_vote() 162 +problem function-size /src/feature/dirauth/dirvote.c:dirvote_compute_consensuses() 164 +problem function-size /src/feature/dirauth/dirvote.c:dirserv_generate_networkstatus_vote_obj() 293 +problem function-size /src/feature/dirauth/guardfraction.c:dirserv_read_guardfraction_file_from_str() 110 +problem function-size /src/feature/dirauth/shared_random.c:should_keep_commit() 110 +problem function-size /src/feature/dirauth/process_descs.c:dirserv_add_descriptor() 125 +problem function-size /src/feature/dirauth/bwauth.c:dirserv_read_measured_bandwidths() 127 +problem function-size /src/feature/dirauth/dsigs_parse.c:networkstatus_parse_detached_signatures() 196 +problem function-size /src/feature/dirauth/voteflags.c:dirserv_compute_performance_thresholds() 172 +problem file-size /src/feature/hs/hs_service.c 4171 +problem function-size /src/feature/hs/hs_cell.c:hs_cell_parse_introduce2() 154 +problem function-size /src/feature/hs/hs_common.c:hs_get_responsible_hsdirs() 104 +problem function-size /src/feature/hs/hs_common.c:hs_get_extend_info_from_lspecs() 101 +problem function-size /src/feature/hs/hs_client.c:send_introduce1() 104 +problem function-size /src/feature/hs/hs_client.c:hs_config_client_authorization() 108 +problem function-size /src/feature/hs/hs_config.c:config_generic_service() 149 +problem function-size /src/feature/hs/hs_cell.c:hs_cell_build_establish_intro() 115 +problem file-size /src/feature/hs/hs_descriptor.c 3108 +problem function-size /src/feature/hs/hs_descriptor.c:desc_encode_v3() 108 +problem function-size /src/feature/hs/hs_descriptor.c:decrypt_desc_layer() 110 +problem function-size /src/feature/hs/hs_descriptor.c:decode_introduction_point() 122 +problem function-size /src/feature/hs/hs_descriptor.c:desc_decode_superencrypted_v3() 109 +problem function-size /src/feature/hs/hs_descriptor.c:desc_decode_encrypted_v3() 109 +problem file-size /src/feature/dirclient/dirclient.c 3215 +problem include-count /src/feature/dirclient/dirclient.c 51 +problem function-size /src/feature/dirclient/dirclient.c:directory_get_from_dirserver() 131 +problem function-size /src/feature/dirclient/dirclient.c:directory_initiate_request() 201 +problem function-size /src/feature/dirclient/dirclient.c:directory_send_command() 241 +problem function-size /src/feature/dirclient/dirclient.c:dir_client_decompress_response_body() 114 +problem function-size /src/feature/dirclient/dirclient.c:connection_dir_client_reached_eof() 189 +problem function-size /src/feature/dirclient/dirclient.c:handle_response_fetch_consensus() 105 +problem function-size /src/feature/rend/rendmid.c:rend_mid_establish_intro_legacy() 104 +problem function-size /src/feature/rend/rendparse.c:rend_parse_v2_service_descriptor() 187 +problem function-size /src/feature/rend/rendparse.c:rend_decrypt_introduction_points() 104 +problem function-size /src/feature/rend/rendparse.c:rend_parse_introduction_points() 131 +problem file-size /src/feature/rend/rendservice.c 4509 +problem function-size /src/feature/rend/rendservice.c:rend_service_prune_list_impl_() 107 +problem function-size /src/feature/rend/rendservice.c:rend_config_service() 164 +problem function-size /src/feature/rend/rendservice.c:rend_service_load_auth_keys() 178 +problem function-size /src/feature/rend/rendservice.c:rend_service_receive_introduction() 332 +problem function-size /src/feature/rend/rendservice.c:rend_service_parse_intro_for_v3() 115 +problem function-size /src/feature/rend/rendservice.c:rend_service_decrypt_intro() 115 +problem function-size /src/feature/rend/rendservice.c:rend_service_intro_has_opened() 126 +problem function-size /src/feature/rend/rendservice.c:rend_service_rendezvous_has_opened() 117 +problem function-size /src/feature/rend/rendservice.c:directory_post_to_hs_dir() 108 +problem function-size /src/feature/rend/rendservice.c:upload_service_descriptor() 111 +problem function-size /src/feature/rend/rendservice.c:rend_consider_services_intro_points() 169 +problem function-size /src/feature/rend/rendcache.c:rend_cache_store_v2_desc_as_client() 193 +problem function-size /src/feature/rend/rendclient.c:rend_client_send_introduction() 220 +problem function-size /src/feature/rend/rendcommon.c:rend_encode_v2_descriptors() 225 +problem function-size /src/feature/nodelist/authcert.c:trusted_dirs_load_certs_from_string() 124 +problem function-size /src/feature/nodelist/authcert.c:authority_certs_fetch_missing() 296 +problem function-size /src/feature/nodelist/microdesc.c:microdesc_cache_rebuild() 134 +problem function-size /src/feature/nodelist/fmt_routerstatus.c:routerstatus_format_entry() 166 +problem include-count /src/feature/nodelist/networkstatus.c 61 +problem function-size /src/feature/nodelist/networkstatus.c:networkstatus_check_consensus_signature() 176 +problem function-size /src/feature/nodelist/networkstatus.c:networkstatus_set_current_consensus() 293 +problem file-size /src/feature/nodelist/routerlist.c 3234 +problem function-size /src/feature/nodelist/routerlist.c:router_rebuild_store() 148 +problem function-size /src/feature/nodelist/routerlist.c:router_add_to_routerlist() 169 +problem function-size /src/feature/nodelist/routerlist.c:routerlist_remove_old_routers() 121 +problem function-size /src/feature/nodelist/routerlist.c:update_consensus_router_descriptor_downloads() 136 +problem function-size /src/feature/nodelist/routerlist.c:update_extrainfo_downloads() 103 +problem function-size /src/feature/nodelist/nodelist.c:compute_frac_paths_available() 187 +problem function-size /src/feature/nodelist/node_select.c:router_pick_directory_server_impl() 123 +problem function-size /src/feature/nodelist/node_select.c:compute_weighted_bandwidths() 205 +problem function-size /src/feature/nodelist/node_select.c:router_pick_trusteddirserver_impl() 114 +problem function-size /src/feature/client/dnsserv.c:evdns_server_callback() 153 +problem function-size /src/feature/client/transports.c:handle_proxy_line() 108 +problem function-size /src/feature/client/transports.c:parse_method_line_helper() 112 +problem function-size /src/feature/client/transports.c:create_managed_proxy_environment() 109 +problem function-size /src/feature/client/bridges.c:rewrite_node_address_for_bridge() 126 +problem function-size /src/feature/client/addressmap.c:addressmap_rewrite() 112 +problem file-size /src/feature/client/entrynodes.c 3817 +problem function-size /src/feature/client/entrynodes.c:entry_guards_upgrade_waiting_circuits() 153 +problem function-size /src/feature/client/entrynodes.c:entry_guard_parse_from_state() 246 +problem function-size /src/feature/client/circpathbias.c:pathbias_measure_close_rate() 108 +problem function-size /src/lib/log/log.c:parse_log_severity_config() 101 +problem function-size /src/lib/math/prob_distr.c:sample_uniform_interval() 145 +problem function-size /src/lib/process/process_win32.c:process_win32_exec() 138 +problem function-size /src/lib/process/process_win32.c:process_win32_create_pipe() 112 +problem function-size /src/lib/process/setuid.c:switch_id() 156 +problem function-size /src/lib/process/restrict.c:set_max_file_descriptors() 102 +problem function-size /src/lib/process/process_unix.c:process_unix_exec() 220 +problem function-size /src/lib/container/smartlist.c:smartlist_bsearch_idx() 109 +problem function-size /src/lib/sandbox/sandbox.c:prot_strings() 104 +problem function-size /src/lib/compress/compress_zstd.c:tor_zstd_compress_process() 126 +problem function-size /src/lib/compress/compress.c:tor_compress_impl() 133 +problem function-size /src/lib/crypt_ops/crypto_rand.c:crypto_strongest_rand_syscall() 102 +problem function-size /src/lib/string/scanf.c:tor_vsscanf() 112 +problem function-size /src/lib/encoding/confline.c:parse_config_line_from_str_verbose() 119 +problem function-size /src/lib/encoding/binascii.c:base64_encode() 107 +problem function-size /src/lib/encoding/cstring.c:unescape_string() 108 +problem function-size /src/lib/tls/tortls_openssl.c:tor_tls_context_new() 171 +problem function-size /src/lib/tls/x509_nss.c:tor_tls_create_certificate_internal() 126 +problem function-size /src/lib/tls/tortls_nss.c:tor_tls_context_new() 147 +problem function-size /src/lib/osinfo/uname.c:get_uname() 116 +problem function-size /src/lib/net/address.c:tor_addr_parse_mask_ports() 198 +problem function-size /src/lib/net/address.c:tor_addr_compare_masked() 111 +problem function-size /src/lib/net/resolve.c:tor_addr_lookup() 110 +problem function-size /src/lib/net/inaddr.c:tor_inet_pton() 107 +problem function-size /src/lib/net/socketpair.c:tor_ersatz_socketpair() 102 +problem function-size /src/lib/fs/dir.c:check_private_dir() 231 diff --git a/scripts/maint/practracker/metrics.py b/scripts/maint/practracker/metrics.py new file mode 100644 index 0000000000..5fa305a868 --- /dev/null +++ b/scripts/maint/practracker/metrics.py @@ -0,0 +1,50 @@ +#!/usr/bin/python + +# Implementation of various source code metrics. +# These are currently ad-hoc string operations and regexps. +# We might want to use a proper static analysis library in the future, if we want to get more advanced metrics. + +import re + +def get_file_len(f): + """Get file length of file""" + for i, l in enumerate(f): + pass + return i + 1 + +def get_include_count(f): + """Get number of #include statements in the file""" + include_count = 0 + for line in f: + if re.match(r' *# *include', line): + include_count += 1 + return include_count + +def get_function_lines(f): + """ + Return iterator which iterates over functions and returns (function name, function lines) + """ + + # Skip lines that look like they are defining functions with these + # names: they aren't real function definitions. + REGEXP_CONFUSE_TERMS = {"MOCK_IMPL", "ENABLE_GCC_WARNINGS", "ENABLE_GCC_WARNING", "DUMMY_TYPECHECK_INSTANCE", + "DISABLE_GCC_WARNING", "DISABLE_GCC_WARNINGS"} + + in_function = False + for lineno, line in enumerate(f): + if not in_function: + # find the start of a function + m = re.match(r'^([a-zA-Z_][a-zA-Z_0-9]*),?\(', line) + if m: + func_name = m.group(1) + if func_name in REGEXP_CONFUSE_TERMS: + continue + func_start = lineno + in_function = True + + else: + # Find the end of a function + if line.startswith("}"): + n_lines = lineno - func_start + in_function = False + yield (func_name, n_lines) diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py new file mode 100755 index 0000000000..08b74c264f --- /dev/null +++ b/scripts/maint/practracker/practracker.py @@ -0,0 +1,149 @@ +#!/usr/bin/python3 + +""" +Best-practices tracker for Tor source code. + +Go through the various .c files and collect metrics about them. If the metrics +violate some of our best practices and they are not found in the optional +exceptions file, then log a problem about them. + +We currently do metrics about file size, function size and number of includes. + +practracker.py should be run with its second argument pointing to the Tor +top-level source directory like this: + $ python3 ./scripts/maint/practracker/practracker.py . + +The exceptions file is meant to be initialized once with the current state of +the source code and then get saved in the repository for ever after: + $ python3 ./scripts/maint/practracker/practracker.py . > ./scripts/maint/practracker/exceptions.txt +""" + +import os, sys + +import metrics +import util +import problem + +# The filename of the exceptions file (it should be placed in the practracker directory) +EXCEPTIONS_FNAME = "./exceptions.txt" + +# Recommended file size +MAX_FILE_SIZE = 3000 # lines +# Recommended function size +MAX_FUNCTION_SIZE = 100 # lines +# Recommended number of #includes +MAX_INCLUDE_COUNT = 50 + +####################################################### + +# ProblemVault singleton +ProblemVault = None + +# The Tor source code topdir +TOR_TOPDIR = None + +####################################################### + +def consider_file_size(fname, f): + """Consider file size issues for 'f' and return True if a new issue was found""" + file_size = metrics.get_file_len(f) + if file_size > MAX_FILE_SIZE: + p = problem.FileSizeProblem(fname, file_size) + return ProblemVault.register_problem(p) + return False + +def consider_includes(fname, f): + """Consider #include issues for 'f' and return True if a new issue was found""" + include_count = metrics.get_include_count(f) + + if include_count > MAX_INCLUDE_COUNT: + p = problem.IncludeCountProblem(fname, include_count) + return ProblemVault.register_problem(p) + return False + +def consider_function_size(fname, f): + """Consider the function sizes for 'f' and return True if a new issue was found""" + found_new_issues = False + + for name, lines in metrics.get_function_lines(f): + # Don't worry about functions within our limits + if lines <= MAX_FUNCTION_SIZE: + continue + + # That's a big function! Issue a problem! + canonical_function_name = "%s:%s()" % (fname, name) + p = problem.FunctionSizeProblem(canonical_function_name, lines) + found_new_issues |= ProblemVault.register_problem(p) + + return found_new_issues + +####################################################### + +def consider_all_metrics(files_list): + """Consider metrics for all files, and return True if new issues were found""" + found_new_issues = False + for fname in files_list: + with open(fname, 'r') as f: + found_new_issues |= consider_metrics_for_file(fname, f) + return found_new_issues + +def consider_metrics_for_file(fname, f): + """ + Consider the various metrics for file with filename 'fname' and file descriptor 'f'. + Return True if we found new issues. + """ + # Strip the useless part of the path + if fname.startswith(TOR_TOPDIR): + fname = fname[len(TOR_TOPDIR):] + + found_new_issues = False + + # Get file length + found_new_issues |= consider_file_size(fname, f) + + # Consider number of #includes + f.seek(0) + found_new_issues |= consider_includes(fname, f) + + # Get function length + f.seek(0) + found_new_issues |= consider_function_size(fname, f) + + return found_new_issues + +def main(): + if (len(sys.argv) != 2): + print("Usage:\n\t$ practracker.py <tor topdir>\n\t(e.g. $ practracker.py ~/tor/)") + return + + global TOR_TOPDIR + TOR_TOPDIR = sys.argv[1] + exceptions_file = os.path.join(TOR_TOPDIR, "scripts/maint/practracker", EXCEPTIONS_FNAME) + + # 1) Get all the .c files we care about + files_list = util.get_tor_c_files(TOR_TOPDIR) + + # 2) Initialize problem vault and load an optional exceptions file so that + # we don't warn about the past + global ProblemVault + ProblemVault = problem.ProblemVault(exceptions_file) + + # 3) Go through all the files and report problems if they are not exceptions + found_new_issues = consider_all_metrics(files_list) + + # If new issues were found, try to give out some advice to the developer on how to resolve it. + if (found_new_issues): + new_issues_str = """\ +FAILURE: practracker found new problems in the code: see warnings above. + +Please fix the problems if you can, and update the exceptions file +({}) if you can't. + +See doc/HACKING/HelpfulTools.md for more information on using practracker.\ +""".format(exceptions_file) + print(new_issues_str) + + sys.exit(found_new_issues) + +if __name__ == '__main__': + main() diff --git a/scripts/maint/practracker/practracker_tests.py b/scripts/maint/practracker/practracker_tests.py new file mode 100755 index 0000000000..cdbab2908e --- /dev/null +++ b/scripts/maint/practracker/practracker_tests.py @@ -0,0 +1,50 @@ +"""Some simple tests for practracker metrics""" + +import unittest + +import StringIO + +import metrics + +function_file = """static void +fun(directory_request_t *req, const char *resource) +{ + time_t if_modified_since = 0; + uint8_t or_diff_from[DIGEST256_LEN]; +} + +static void +fun(directory_request_t *req, + const char *resource) +{ + time_t if_modified_since = 0; + uint8_t or_diff_from[DIGEST256_LEN]; +} + +MOCK_IMPL(void, +fun,( + uint8_t dir_purpose, + uint8_t router_purpose, + const char *resource, + int pds_flags, + download_want_authority_t want_authority)) +{ + const routerstatus_t *rs = NULL; + const or_options_t *options = get_options(); +} +""" + +class TestFunctionLength(unittest.TestCase): + def test_function_length(self): + funcs = StringIO.StringIO(function_file) + # All functions should have length 2 + for name, lines in metrics.function_lines(funcs): + self.assertEqual(name, "fun") + + funcs.seek(0) + + for name, lines in metrics.function_lines(funcs): + self.assertEqual(lines, 2) + +if __name__ == '__main__': + unittest.main() diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py new file mode 100644 index 0000000000..61420fb785 --- /dev/null +++ b/scripts/maint/practracker/problem.py @@ -0,0 +1,138 @@ +""" +In this file we define a ProblemVault class where we store all the +exceptions and all the problems we find with the code. + +The ProblemVault is capable of registering problems and also figuring out if a +problem is worse than a registered exception so that it only warns when things +get worse. +""" + +import os.path +import sys + +class ProblemVault(object): + """ + Singleton where we store the various new problems we + found in the code, and also the old problems we read from the exception + file. + """ + def __init__(self, exception_fname): + # Exception dictionary: { problem.key() : Problem object } + self.exceptions = {} + + try: + with open(exception_fname, 'r') as exception_f: + self.register_exceptions(exception_f) + except IOError: + print("No exception file provided", file=sys.stderr) + + def register_exceptions(self, exception_file): + # Register exceptions + for line in exception_file: + problem = get_old_problem_from_exception_str(line) + if problem is None: + continue + + # Fail if we see dup exceptions. There is really no reason to have dup exceptions. + if problem.key() in self.exceptions: + print("Duplicate exceptions lines found in exception file:\n\t{}\n\t{}\nAborting...".format(problem, self.exceptions[problem.key()]), + file=sys.stderr) + sys.exit(1) + + self.exceptions[problem.key()] = problem + #print "Registering exception: %s" % problem + + def register_problem(self, problem): + """ + Register this problem to the problem value. Return True if it was a new + problem or it worsens an already existing problem. + """ + # This is a new problem, print it + if problem.key() not in self.exceptions: + print(problem) + return True + + # If it's an old problem, we don't warn if the situation got better + # (e.g. we went from 4k LoC to 3k LoC), but we do warn if the + # situation worsened (e.g. we went from 60 includes to 80). + if problem.is_worse_than(self.exceptions[problem.key()]): + print(problem) + return True + + return False + +class Problem(object): + """ + A generic problem in our source code. See the subclasses below for the + specific problems we are trying to tackle. + """ + def __init__(self, problem_type, problem_location, metric_value): + self.problem_location = problem_location + self.metric_value = int(metric_value) + self.problem_type = problem_type + + def is_worse_than(self, other_problem): + """Return True if this is a worse problem than other_problem""" + if self.metric_value > other_problem.metric_value: + return True + return False + + def key(self): + """Generate a unique key that describes this problem that can be used as a dictionary key""" + # Problem location is a filesystem path, so we need to normalize this + # across platforms otherwise same paths are not gonna match. + canonical_location = os.path.normcase(self.problem_location) + return "%s:%s" % (canonical_location, self.problem_type) + + def __str__(self): + return "problem %s %s %s" % (self.problem_type, self.problem_location, self.metric_value) + +class FileSizeProblem(Problem): + """ + Denotes a problem with the size of a .c file. + + The 'problem_location' is the filesystem path of the .c file, and the + 'metric_value' is the number of lines in the .c file. + """ + def __init__(self, problem_location, metric_value): + super(FileSizeProblem, self).__init__("file-size", problem_location, metric_value) + +class IncludeCountProblem(Problem): + """ + Denotes a problem with the number of #includes in a .c file. + + The 'problem_location' is the filesystem path of the .c file, and the + 'metric_value' is the number of #includes in the .c file. + """ + def __init__(self, problem_location, metric_value): + super(IncludeCountProblem, self).__init__("include-count", problem_location, metric_value) + +class FunctionSizeProblem(Problem): + """ + Denotes a problem with a size of a function in a .c file. + + The 'problem_location' is "<path>:<function>()" where <path> is the + filesystem path of the .c file and <function> is the name of the offending + function. + + The 'metric_value' is the size of the offending function in lines. + """ + def __init__(self, problem_location, metric_value): + super(FunctionSizeProblem, self).__init__("function-size", problem_location, metric_value) + +def get_old_problem_from_exception_str(exception_str): + try: + _, problem_type, problem_location, metric_value = exception_str.split(" ") + except ValueError: + return None + + if problem_type == "file-size": + return FileSizeProblem(problem_location, metric_value) + elif problem_type == "include-count": + return IncludeCountProblem(problem_location, metric_value) + elif problem_type == "function-size": + return FunctionSizeProblem(problem_location, metric_value) + else: +# print("Unknown exception line '{}'".format(exception_str)) + return None + diff --git a/scripts/maint/practracker/util.py b/scripts/maint/practracker/util.py new file mode 100644 index 0000000000..63de72d5a3 --- /dev/null +++ b/scripts/maint/practracker/util.py @@ -0,0 +1,27 @@ +import os + +# We don't want to run metrics for unittests, automatically-generated C files, +# external libraries or git leftovers. +EXCLUDE_SOURCE_DIRS = {"/src/test/", "/src/trunnel/", "/src/ext/", "/.git/"} + +def get_tor_c_files(tor_topdir): + """ + Return a list with the .c filenames we want to get metrics of. + """ + files_list = [] + + for root, directories, filenames in os.walk(tor_topdir): + for filename in filenames: + # We only care about .c files + if not filename.endswith(".c"): + continue + + # Exclude the excluded paths + full_path = os.path.join(root,filename) + if any(os.path.normcase(exclude_dir) in full_path for exclude_dir in EXCLUDE_SOURCE_DIRS): + continue + + files_list.append(full_path) + + return files_list + diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index ba6bfe1f53..2895d49e18 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -224,25 +224,20 @@ circpad_machine_current_state(const circpad_machine_state_t *mi) } /** - * Calculate the lower bound of a histogram bin. The upper bound - * is obtained by calling this function with bin+1, and subtracting 1. + * Get the lower bound of a histogram bin. * - * The 0th bin has a special value -- it only represents start_usec. - * This is so we can specify a probability on 0-delay values. + * You can obtain the upper bound using histogram_get_bin_upper_bound(). * - * After bin 0, bins are exponentially spaced, so that each subsequent - * bin is twice as large as the previous. This is done so that higher - * time resolution is given to lower time values. - * - * The infinity bin is a the last bin in the array (histogram_len-1). - * It has a usec value of CIRCPAD_DELAY_INFINITE (UINT32_MAX). + * This function can also be called with 'bin' set to a value equal or greater + * than histogram_len in which case the infinity bin is chosen and + * CIRCPAD_DELAY_INFINITE is returned. */ STATIC circpad_delay_t circpad_histogram_bin_to_usec(const circpad_machine_state_t *mi, circpad_hist_index_t bin) { const circpad_state_t *state = circpad_machine_current_state(mi); - circpad_delay_t start_usec; + circpad_delay_t rtt_add_usec = 0; /* Our state should have been checked to be non-null by the caller * (circpad_machine_remove_token()) */ @@ -250,27 +245,29 @@ circpad_histogram_bin_to_usec(const circpad_machine_state_t *mi, return CIRCPAD_DELAY_INFINITE; } - if (state->use_rtt_estimate) - start_usec = mi->rtt_estimate_usec+state->start_usec; - else - start_usec = state->start_usec; - - if (bin >= CIRCPAD_INFINITY_BIN(state)) + /* The infinity bin has an upper bound of infinity, so make sure we return + * that if they ask for it. */ + if (bin > CIRCPAD_INFINITY_BIN(mi)) { return CIRCPAD_DELAY_INFINITE; + } - if (bin == 0) - return start_usec; + /* If we are using an RTT estimate, consider it as well. */ + if (state->use_rtt_estimate) { + rtt_add_usec = mi->rtt_estimate_usec; + } - if (bin == 1) - return start_usec+1; + return state->histogram_edges[bin] + rtt_add_usec; +} - /* The bin widths double every index, so that we can have more resolution - * for lower time values in the histogram. */ - const circpad_time_t bin_width_exponent = - 1 << (CIRCPAD_INFINITY_BIN(state) - bin); - return (circpad_delay_t)MIN(start_usec + - state->range_usec/bin_width_exponent, - CIRCPAD_DELAY_INFINITE); +/** + * Like circpad_histogram_bin_to_usec() but return the upper bound of bin. + * (The upper bound is included in the bin.) + */ +STATIC circpad_delay_t +histogram_get_bin_upper_bound(const circpad_machine_state_t *mi, + circpad_hist_index_t bin) +{ + return circpad_histogram_bin_to_usec(mi, bin+1) - 1; } /** Return the midpoint of the histogram bin <b>bin_index</b>. */ @@ -279,8 +276,7 @@ circpad_get_histogram_bin_midpoint(const circpad_machine_state_t *mi, int bin_index) { circpad_delay_t left_bound = circpad_histogram_bin_to_usec(mi, bin_index); - circpad_delay_t right_bound = - circpad_histogram_bin_to_usec(mi, bin_index+1)-1; + circpad_delay_t right_bound = histogram_get_bin_upper_bound(mi, bin_index); return left_bound + (right_bound - left_bound)/2; } @@ -289,19 +285,17 @@ circpad_get_histogram_bin_midpoint(const circpad_machine_state_t *mi, * Return the bin that contains the usec argument. * "Contains" is defined as us in [lower, upper). * - * This function will never return the infinity bin (histogram_len-1), - * in order to simplify the rest of the code. - * - * This means that technically the last bin (histogram_len-2) - * has range [start_usec+range_usec, CIRCPAD_DELAY_INFINITE]. + * This function will never return the infinity bin (histogram_len-1), in order + * to simplify the rest of the code, so if a usec is provided that falls above + * the highest non-infinity bin, that bin index will be returned. */ STATIC circpad_hist_index_t circpad_histogram_usec_to_bin(const circpad_machine_state_t *mi, circpad_delay_t usec) { const circpad_state_t *state = circpad_machine_current_state(mi); - circpad_delay_t start_usec; - int32_t bin; /* Larger than return type to properly clamp overflow */ + circpad_delay_t rtt_add_usec = 0; + circpad_hist_index_t bin; /* Our state should have been checked to be non-null by the caller * (circpad_machine_remove_token()) */ @@ -309,34 +303,25 @@ circpad_histogram_usec_to_bin(const circpad_machine_state_t *mi, return 0; } - if (state->use_rtt_estimate) - start_usec = mi->rtt_estimate_usec+state->start_usec; - else - start_usec = state->start_usec; - - /* The first bin (#0) has zero width and starts (and ends) at start_usec. */ - if (usec <= start_usec) - return 0; - - if (usec == start_usec+1) - return 1; + /* If we are using an RTT estimate, consider it as well. */ + if (state->use_rtt_estimate) { + rtt_add_usec = mi->rtt_estimate_usec; + } - const circpad_time_t histogram_range_usec = state->range_usec; - /* We need to find the bin corresponding to our position in the range. - * Since bins are exponentially spaced in powers of two, we need to - * take the log2 of our position in histogram_range_usec. However, - * since tor_log2() returns the floor(log2(u64)), we have to adjust - * it to behave like ceil(log2(u64)). This is verified in our tests - * to properly invert the operation done in - * circpad_histogram_bin_to_usec(). */ - bin = CIRCPAD_INFINITY_BIN(state) - - tor_log2(2*histogram_range_usec/(usec-start_usec+1)); + /* Walk through the bins and check the upper bound of each bin, if 'usec' is + * less-or-equal to that, return that bin. If rtt_estimate is enabled then + * add that to the upper bound of each bin. + * + * We don't want to return the infinity bin here, so don't go there. */ + for (bin = 0 ; bin < CIRCPAD_INFINITY_BIN(state) ; bin++) { + if (usec <= histogram_get_bin_upper_bound(mi, bin) + rtt_add_usec) { + return bin; + } + } - /* Clamp the return value to account for timevals before the start - * of bin 0, or after the last bin. Don't return the infinity bin - * index. */ - bin = MIN(MAX(bin, 1), CIRCPAD_INFINITY_BIN(state)-1); - return bin; + /* We don't want to return the infinity bin here, so if we still didn't find + * the right bin, return the highest non-infinity bin */ + return CIRCPAD_INFINITY_BIN(state)-1; } /** @@ -398,20 +383,22 @@ circpad_choose_state_length(circpad_machine_state_t *mi) /** * Sample a value from our iat_dist, and clamp it safely * to circpad_delay_t. + * + * Before returning, add <b>delay_shift</b> (can be zero) to the sampled value. */ static circpad_delay_t circpad_distribution_sample_iat_delay(const circpad_state_t *state, - circpad_delay_t start_usec) + circpad_delay_t delay_shift) { double val = circpad_distribution_sample(state->iat_dist); /* These comparisons are safe, because the output is in the range * [0, 2**32), and double has a precision of 53 bits. */ val = MAX(0, val); - val = MIN(val, state->range_usec); + val = MIN(val, state->dist_max_sample_usec); - /* This addition is exact: val is at most 2**32-1, start_usec + /* This addition is exact: val is at most 2**32-1, min_delay * is at most 2**32-1, and doubles have a precision of 53 bits. */ - val += start_usec; + val += delay_shift; /* Clamp the distribution at infinite delay val */ return (circpad_delay_t)MIN(tor_llround(val), CIRCPAD_DELAY_INFINITE); @@ -431,7 +418,6 @@ circpad_machine_sample_delay(circpad_machine_state_t *mi) const circpad_hist_token_t *histogram = NULL; circpad_hist_index_t curr_bin = 0; circpad_delay_t bin_start, bin_end; - circpad_delay_t start_usec; /* These three must all be larger than circpad_hist_token_t, because * we sum several circpad_hist_token_t values across the histogram */ uint64_t curr_weight = 0; @@ -440,14 +426,12 @@ circpad_machine_sample_delay(circpad_machine_state_t *mi) tor_assert(state); - if (state->use_rtt_estimate) - start_usec = mi->rtt_estimate_usec+state->start_usec; - else - start_usec = state->start_usec; - if (state->iat_dist.type != CIRCPAD_DIST_NONE) { /* Sample from a fixed IAT distribution and return */ - return circpad_distribution_sample_iat_delay(state, start_usec); + circpad_delay_t iat_delay_shift = state->use_rtt_estimate ? + mi->rtt_estimate_usec + state->dist_added_shift_usec : + state->dist_added_shift_usec; + return circpad_distribution_sample_iat_delay(state, iat_delay_shift); } else if (state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE) { /* We have a mutable histogram. Do basic sanity check and apply: */ if (BUG(!mi->histogram) || @@ -512,16 +496,13 @@ circpad_machine_sample_delay(circpad_machine_state_t *mi) * function below samples from [bin_start, bin_end) */ bin_end = circpad_histogram_bin_to_usec(mi, curr_bin+1); - /* Truncate the high bin in case it's the infinity bin: - * Don't actually schedule an "infinite"-1 delay */ - bin_end = MIN(bin_end, start_usec+state->range_usec); - - // Sample uniformly between histogram[i] to histogram[i+1]-1, - // but no need to sample if they are the same timeval (aka bin 0 or bin 1). - if (bin_end <= bin_start+1) + /* Bin edges are monotonically increasing so this is a bug. Handle it. */ + if (BUG(bin_start > bin_end)) { return bin_start; - else - return (circpad_delay_t)crypto_rand_uint64_range(bin_start, bin_end); + } + + /* Sample randomly from within the bin width */ + return (circpad_delay_t)crypto_rand_uint64_range(bin_start, bin_end); } /** @@ -627,7 +608,7 @@ circpad_machine_first_higher_index(const circpad_machine_state_t *mi, /* Don't remove from the infinity bin */ for (; bin < CIRCPAD_INFINITY_BIN(mi); bin++) { if (mi->histogram[bin] && - circpad_histogram_bin_to_usec(mi, bin+1) > target_bin_usec) { + histogram_get_bin_upper_bound(mi, bin) >= target_bin_usec) { return bin; } } @@ -2083,16 +2064,92 @@ circpad_setup_machine_on_circ(circuit_t *on_circ, on_circ->padding_machine[machine->machine_index] = machine; } -/* These padding machines are only used for tests pending #28634. */ #ifdef TOR_UNIT_TESTS +/** Validate a single state of a padding machine */ +static bool +padding_machine_state_is_valid(const circpad_state_t *state) +{ + int b; + uint32_t tokens_count = 0; + circpad_delay_t prev_bin_edge = 0; + + /* We only validate histograms */ + if (!state->histogram_len) { + return true; + } + + /* We need at least two bins in a histogram */ + if (state->histogram_len < 2) { + log_warn(LD_GENERAL, "You can't have a histogram with less than 2 bins"); + return false; + } + + /* For each machine state, if it's a histogram, make sure all the + * histogram edges are well defined (i.e. are strictly monotonic). */ + for (b = 0 ; b < state->histogram_len ; b++) { + /* Check that histogram edges are strictly increasing. Ignore the first + * edge since it can be zero. */ + if (prev_bin_edge >= state->histogram_edges[b] && b > 0) { + log_warn(LD_GENERAL, "Histogram edges are not increasing [%u/%u]", + prev_bin_edge, state->histogram_edges[b]); + return false; + } + + prev_bin_edge = state->histogram_edges[b]; + + /* Also count the number of tokens as we go through the histogram states */ + tokens_count += state->histogram[b]; + } + /* Verify that the total number of tokens is correct */ + if (tokens_count != state->histogram_total_tokens) { + log_warn(LD_GENERAL, "Histogram token count is wrong [%u/%u]", + tokens_count, state->histogram_total_tokens); + return false; + } + + return true; +} + +/** Basic validation of padding machine */ +static bool +padding_machine_is_valid(const circpad_machine_spec_t *machine) +{ + int i; + + /* Validate the histograms of the padding machine */ + for (i = 0 ; i < machine->num_states ; i++) { + if (!padding_machine_state_is_valid(&machine->states[i])) { + return false; + } + } + + return true; +} + +/* Validate and register <b>machine</b> into <b>machine_list</b>. If + * <b>machine_list</b> is NULL, then just validate. */ +STATIC void +register_padding_machine(circpad_machine_spec_t *machine, + smartlist_t *machine_list) +{ + if (!padding_machine_is_valid(machine)) { + log_warn(LD_GENERAL, "Machine #%u is invalid. Ignoring.", + machine->machine_num); + return; + } + + if (machine_list) { + smartlist_add(machine_list, machine); + } +} + +/* These padding machines are only used for tests pending #28634. */ static void circpad_circ_client_machine_init(void) { circpad_machine_spec_t *circ_client_machine = tor_malloc_zero(sizeof(circpad_machine_spec_t)); - // XXX: Better conditions for merge.. Or disable this machine in - // merge? circ_client_machine->conditions.min_hops = 2; circ_client_machine->conditions.state_mask = CIRCPAD_CIRC_BUILDING|CIRCPAD_CIRC_OPENED|CIRCPAD_CIRC_HAS_RELAY_EARLY; @@ -2124,19 +2181,20 @@ circpad_circ_client_machine_init(void) circ_client_machine->states[CIRCPAD_STATE_BURST].token_removal = CIRCPAD_TOKEN_REMOVAL_CLOSEST; - // FIXME: Tune this histogram circ_client_machine->states[CIRCPAD_STATE_BURST].histogram_len = 2; - circ_client_machine->states[CIRCPAD_STATE_BURST].start_usec = 500; - circ_client_machine->states[CIRCPAD_STATE_BURST].range_usec = 1000000; + circ_client_machine->states[CIRCPAD_STATE_BURST].histogram_edges[0]= 500; + circ_client_machine->states[CIRCPAD_STATE_BURST].histogram_edges[1]= 1000000; + /* We have 5 tokens in the histogram, which means that all circuits will look * like they have 7 hops (since we start this machine after the second hop, * and tokens are decremented for any valid hops, and fake extends are * used after that -- 2+5==7). */ circ_client_machine->states[CIRCPAD_STATE_BURST].histogram[0] = 5; + circ_client_machine->states[CIRCPAD_STATE_BURST].histogram_total_tokens = 5; circ_client_machine->machine_num = smartlist_len(origin_padding_machines); - smartlist_add(origin_padding_machines, circ_client_machine); + register_padding_machine(circ_client_machine, origin_padding_machines); } static void @@ -2183,8 +2241,9 @@ circpad_circ_responder_machine_init(void) circ_responder_machine->states[CIRCPAD_STATE_BURST].use_rtt_estimate = 1; /* The histogram is 2 bins: an empty one, and infinity */ circ_responder_machine->states[CIRCPAD_STATE_BURST].histogram_len = 2; - circ_responder_machine->states[CIRCPAD_STATE_BURST].start_usec = 5000; - circ_responder_machine->states[CIRCPAD_STATE_BURST].range_usec = 1000000; + circ_responder_machine->states[CIRCPAD_STATE_BURST].histogram_edges[0]= 500; + circ_responder_machine->states[CIRCPAD_STATE_BURST].histogram_edges[1] = + 1000000; /* During burst state we wait forever for padding to arrive. We are waiting for a padding cell from the client to come in, so that we @@ -2215,8 +2274,14 @@ circpad_circ_responder_machine_init(void) before you send a padding response */ circ_responder_machine->states[CIRCPAD_STATE_GAP].use_rtt_estimate = 1; circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_len = 6; - circ_responder_machine->states[CIRCPAD_STATE_GAP].start_usec = 5000; - circ_responder_machine->states[CIRCPAD_STATE_GAP].range_usec = 1000000; + /* Specify histogram bins */ + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[0]= 500; + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[1]= 1000; + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[2]= 2000; + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[3]= 4000; + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[4]= 8000; + circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_edges[5]= 16000; + /* Specify histogram tokens */ circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram[0] = 0; circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram[1] = 1; circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram[2] = 2; @@ -2224,11 +2289,12 @@ circpad_circ_responder_machine_init(void) circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram[4] = 1; /* Total number of tokens */ circ_responder_machine->states[CIRCPAD_STATE_GAP].histogram_total_tokens = 6; + circ_responder_machine->states[CIRCPAD_STATE_GAP].token_removal = CIRCPAD_TOKEN_REMOVAL_CLOSEST_USEC; circ_responder_machine->machine_num = smartlist_len(relay_padding_machines); - smartlist_add(relay_padding_machines, circ_responder_machine); + register_padding_machine(circ_responder_machine, relay_padding_machines); } #endif diff --git a/src/core/or/circuitpadding.h b/src/core/or/circuitpadding.h index 92fd4fc2d5..54039fa59a 100644 --- a/src/core/or/circuitpadding.h +++ b/src/core/or/circuitpadding.h @@ -228,13 +228,18 @@ typedef uint16_t circpad_statenum_t; #define CIRCPAD_STATENUM_MAX (UINT16_MAX) /** A histogram is used to sample padding delays given a machine state. This - * constant defines the maximum histogram width (i.e. the max number of bins) + * constant defines the maximum histogram width (i.e. the max number of bins). * - * Each histogram bin is twice as large as the previous. Two exceptions: The - * first bin has zero width (which means that minimum delay is applied to the - * next padding cell), and the last bin (infinity bin) has infinite width - * (which means that the next padding cell will be delayed infinitely). */ -#define CIRCPAD_MAX_HISTOGRAM_LEN (sizeof(circpad_delay_t)*8 + 1) + * The current limit is arbitrary and could be raised if there is a need, + * however too many bins will be hard to serialize in the future. + * + * Memory concerns are not so great here since the corresponding histogram and + * histogram_edges arrays are global and not per-circuit. + * + * If we ever upgrade this to a value that can't be represented by 8-bits we + * also need to upgrade circpad_hist_index_t. + */ +#define CIRCPAD_MAX_HISTOGRAM_LEN (100) /** * A state of a padding state machine. The information here are immutable and @@ -248,38 +253,60 @@ typedef uint16_t circpad_statenum_t; * or the consensus. */ typedef struct circpad_state_t { - /** If a histogram is used for this state, this specifies the number of bins - * of this histogram. Histograms must have at least 2 bins. + /** + * If a histogram is used for this state, this specifies the number of bins + * of this histogram. Histograms must have at least 2 bins. + * + * In particular, the following histogram: + * + * Tokens + * + + * 10 | +----+ + * 9 | | | +---------+ + * 8 | | | | | + * 7 | | | +-----+ | + * 6 +----+ Bin+-----+ | +---------------+ + * 5 | | #1 | | | | | + * | Bin| | Bin | Bin | Bin #4 | Bin #5 | + * | #0 | | #2 | #3 | | (infinity bin)| + * | | | | | | | + * | | | | | | | + * 0 +----+----+-----+-----+---------+---------------+ + * 0 100 200 350 500 1000 ∞ microseconds * - * If a delay probability distribution is used for this state, this is set - * to 0. */ + * would be specified the following way: + * histogram_len = 6; + * histogram[] = { 6, 10, 6, 7, 9, 6 } + * histogram_edges[] = { 0, 100, 200, 350, 500, 1000 } + * + * The final bin is called the "infinity bin" and if it's chosen we don't + * schedule any padding. The infinity bin is strange because its lower edge + * is the max value of possible non-infinite delay allowed by this histogram, + * and its upper edge is CIRCPAD_DELAY_INFINITE. You can tell if the infinity + * bin is chosen by inspecting its bin index or inspecting its upper edge. + * + * If a delay probability distribution is used for this state, this is set + * to 0. */ circpad_hist_index_t histogram_len; /** The histogram itself: an array of uint16s of tokens, whose - * widths are exponentially spaced, in microseconds */ + * widths are exponentially spaced, in microseconds. + * + * This array must have histogram_len elements that are strictly + * monotonically increasing. */ circpad_hist_token_t histogram[CIRCPAD_MAX_HISTOGRAM_LEN]; + /* The histogram bin edges in usec. + * + * Each element of this array specifies the left edge of the corresponding + * bin. The rightmost edge is always infinity and is not specified in this + * array. + * + * This array must have histogram_len elements. */ + circpad_delay_t histogram_edges[CIRCPAD_MAX_HISTOGRAM_LEN+1]; /** Total number of tokens in this histogram. This is a constant and is *not* * decremented every time we spend a token. It's used for initializing and * refilling the histogram. */ uint32_t histogram_total_tokens; - /** Minimum padding delay of this state in microseconds. - * - * If histograms are used, this is the left (and right) bound of the first - * bin (since it has zero width). - * - * If a delay probability distribution is used, this represents the minimum - * delay we can sample from the distribution. - */ - circpad_delay_t start_usec; - - /** If histograms are used, this is the width of the whole histogram in - * microseconds, and it's used to calculate individual bin width. - * - * If a delay probability distribution is used, this is used as the max - * delay we can sample from the distribution. - */ - circpad_delay_t range_usec; - /** * Represents a delay probability distribution (aka IAT distribution). It's a * parametrized way of encoding inter-packet delay information in @@ -292,6 +319,16 @@ typedef struct circpad_state_t { * results of sampling from this distribution (range_sec is used as a max). */ circpad_distribution_t iat_dist; + /* If a delay probability distribution is used, this is used as the max + * value we can sample from the distribution. However, RTT measurements and + * dist_added_shift gets applied on top of this value to derive the final + * padding delay. */ + circpad_delay_t dist_max_sample_usec; + /* If a delay probability distribution is used and this is set, we will add + * this value on top of the value sampled from the IAT distribution to + * derive the final padding delay (We also add the RTT measurement if it's + * enabled.). */ + circpad_delay_t dist_added_shift_usec; /** * The length dist is a parameterized way of encoding how long this @@ -686,9 +723,17 @@ circpad_send_command_to_hop,(struct origin_circuit_t *circ, uint8_t hopnum, uint8_t relay_command, const uint8_t *payload, ssize_t payload_len)); +STATIC circpad_delay_t +histogram_get_bin_upper_bound(const circpad_machine_state_t *mi, + circpad_hist_index_t bin); + #ifdef TOR_UNIT_TESTS extern smartlist_t *origin_padding_machines; extern smartlist_t *relay_padding_machines; + +STATIC void +register_padding_machine(circpad_machine_spec_t *machine, + smartlist_t *machine_list); #endif #endif diff --git a/src/feature/dirauth/shared_random.c b/src/feature/dirauth/shared_random.c index 34b2283250..137c49800f 100644 --- a/src/feature/dirauth/shared_random.c +++ b/src/feature/dirauth/shared_random.c @@ -120,8 +120,8 @@ static const char sr_flag_ns_str[] = "shared-rand-participate"; static int32_t num_srv_agreements_from_vote; /* Return a heap allocated copy of the SRV <b>orig</b>. */ -STATIC sr_srv_t * -srv_dup(const sr_srv_t *orig) +sr_srv_t * +sr_srv_dup(const sr_srv_t *orig) { sr_srv_t *duplicate = NULL; @@ -1253,8 +1253,8 @@ sr_act_post_consensus(const networkstatus_t *consensus) * decided by the majority. */ sr_state_unset_fresh_srv(); /* Set the SR values from the given consensus. */ - sr_state_set_previous_srv(srv_dup(consensus->sr_info.previous_srv)); - sr_state_set_current_srv(srv_dup(consensus->sr_info.current_srv)); + sr_state_set_previous_srv(sr_srv_dup(consensus->sr_info.previous_srv)); + sr_state_set_current_srv(sr_srv_dup(consensus->sr_info.current_srv)); } /* Prepare our state so that it's ready for the next voting period. */ diff --git a/src/feature/dirauth/shared_random.h b/src/feature/dirauth/shared_random.h index 25d95ebbc7..0b45ad1ed7 100644 --- a/src/feature/dirauth/shared_random.h +++ b/src/feature/dirauth/shared_random.h @@ -154,6 +154,7 @@ const char *sr_commit_get_rsa_fpr(const sr_commit_t *commit) void sr_compute_srv(void); sr_commit_t *sr_generate_our_commit(time_t timestamp, const authority_cert_t *my_rsa_cert); +sr_srv_t *sr_srv_dup(const sr_srv_t *orig); #ifdef SHARED_RANDOM_PRIVATE @@ -172,7 +173,6 @@ STATIC sr_srv_t *get_majority_srv_from_votes(const smartlist_t *votes, int current); STATIC void save_commit_to_state(sr_commit_t *commit); -STATIC sr_srv_t *srv_dup(const sr_srv_t *orig); STATIC int commitments_are_the_same(const sr_commit_t *commit_one, const sr_commit_t *commit_two); STATIC int commit_is_authoritative(const sr_commit_t *commit, diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index 92f0b3e737..a7b7480edd 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -102,6 +102,8 @@ static const config_format_t state_format = { &state_extra_var, }; +static void state_query_del_(sr_state_object_t obj_type, void *data); + /* Return a string representation of a protocol phase. */ STATIC const char * get_phase_str(sr_phase_t phase) @@ -834,6 +836,9 @@ state_query_get_commit(const char *rsa_fpr) static void * state_query_get_(sr_state_object_t obj_type, const void *data) { + if (BUG(!sr_state)) + return NULL; + void *obj = NULL; switch (obj_type) { @@ -862,23 +867,44 @@ state_query_get_(sr_state_object_t obj_type, const void *data) } /* Helper function: This handles the PUT state action using an - * <b>obj_type</b> and <b>data</b> needed for the action. */ + * <b>obj_type</b> and <b>data</b> needed for the action. + * PUT frees the previous data before replacing it, if needed. */ static void state_query_put_(sr_state_object_t obj_type, void *data) { + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_COMMIT: { sr_commit_t *commit = data; tor_assert(commit); + /* commit_add_to_state() frees the old commit, if there is one */ commit_add_to_state(commit, sr_state); break; } case SR_STATE_OBJ_CURSRV: - sr_state->current_srv = (sr_srv_t *) data; + /* Check if the new pointer is the same as the old one: if it is, it's + * probably a bug. The caller may have confused current and previous, + * or they may have forgotten to sr_srv_dup(). + * Putting NULL multiple times is allowed. */ + if (!BUG(data && sr_state->current_srv == (sr_srv_t *) data)) { + /* We own the old SRV, so we need to free it. */ + state_query_del_(SR_STATE_OBJ_CURSRV, NULL); + sr_state->current_srv = (sr_srv_t *) data; + } break; case SR_STATE_OBJ_PREVSRV: - sr_state->previous_srv = (sr_srv_t *) data; + /* Check if the new pointer is the same as the old one: if it is, it's + * probably a bug. The caller may have confused current and previous, + * or they may have forgotten to sr_srv_dup(). + * Putting NULL multiple times is allowed. */ + if (!BUG(data && sr_state->previous_srv == (sr_srv_t *) data)) { + /* We own the old SRV, so we need to free it. */ + state_query_del_(SR_STATE_OBJ_PREVSRV, NULL); + sr_state->previous_srv = (sr_srv_t *) data; + } break; case SR_STATE_OBJ_VALID_AFTER: sr_state->valid_after = *((time_t *) data); @@ -898,6 +924,9 @@ state_query_put_(sr_state_object_t obj_type, void *data) static void state_query_del_all_(sr_state_object_t obj_type) { + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_COMMIT: { @@ -926,6 +955,9 @@ state_query_del_(sr_state_object_t obj_type, void *data) { (void) data; + if (BUG(!sr_state)) + return; + switch (obj_type) { case SR_STATE_OBJ_PREVSRV: tor_free(sr_state->previous_srv); @@ -986,7 +1018,7 @@ state_query(sr_state_action_t action, sr_state_object_t obj_type, /* Delete the current SRV value from the state freeing it and the value is set * to NULL meaning empty. */ -static void +STATIC void state_del_current_srv(void) { state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_CURSRV, NULL, NULL); @@ -994,22 +1026,22 @@ state_del_current_srv(void) /* Delete the previous SRV value from the state freeing it and the value is * set to NULL meaning empty. */ -static void +STATIC void state_del_previous_srv(void) { state_query(SR_STATE_ACTION_DEL, SR_STATE_OBJ_PREVSRV, NULL, NULL); } -/* Rotate SRV value by freeing the previous value, assigning the current - * value to the previous one and nullifying the current one. */ +/* Rotate SRV value by setting the previous SRV to the current SRV, and + * clearing the current SRV. */ STATIC void state_rotate_srv(void) { /* First delete previous SRV from the state. Object will be freed. */ state_del_previous_srv(); - /* Set previous SRV with the current one. */ - sr_state_set_previous_srv(sr_state_get_current_srv()); - /* Nullify the current srv. */ + /* Set previous SRV to a copy of the current one. */ + sr_state_set_previous_srv(sr_srv_dup(sr_state_get_current_srv())); + /* Free and NULL the current srv. */ sr_state_set_current_srv(NULL); } @@ -1030,7 +1062,9 @@ sr_state_get_phase(void) return *(sr_phase_t *) ptr; } -/* Return the previous SRV value from our state. Value CAN be NULL. */ +/* Return the previous SRV value from our state. Value CAN be NULL. + * The state object owns the SRV, so the calling code should not free the SRV. + * Use sr_srv_dup() if you want to keep a copy of the SRV. */ const sr_srv_t * sr_state_get_previous_srv(void) { @@ -1049,7 +1083,9 @@ sr_state_set_previous_srv(const sr_srv_t *srv) NULL); } -/* Return the current SRV value from our state. Value CAN be NULL. */ +/* Return the current SRV value from our state. Value CAN be NULL. + * The state object owns the SRV, so the calling code should not free the SRV. + * Use sr_srv_dup() if you want to keep a copy of the SRV. */ const sr_srv_t * sr_state_get_current_srv(void) { diff --git a/src/feature/dirauth/shared_random_state.h b/src/feature/dirauth/shared_random_state.h index 35626be3f6..08f999f9d4 100644 --- a/src/feature/dirauth/shared_random_state.h +++ b/src/feature/dirauth/shared_random_state.h @@ -140,6 +140,8 @@ STATIC int is_phase_transition(sr_phase_t next_phase); STATIC void set_sr_phase(sr_phase_t phase); STATIC sr_state_t *get_sr_state(void); +STATIC void state_del_previous_srv(void); +STATIC void state_del_current_srv(void); #endif /* defined(TOR_UNIT_TESTS) */ diff --git a/src/feature/hs/hs_cell.c b/src/feature/hs/hs_cell.c index 597982b34e..1dae9c79c1 100644 --- a/src/feature/hs/hs_cell.c +++ b/src/feature/hs/hs_cell.c @@ -756,7 +756,14 @@ hs_cell_parse_introduce2(hs_cell_introduce2_data_t *data, idx < trn_cell_introduce_encrypted_get_nspec(enc_cell); idx++) { link_specifier_t *lspec = trn_cell_introduce_encrypted_get_nspecs(enc_cell, idx); - smartlist_add(data->link_specifiers, hs_link_specifier_dup(lspec)); + if (BUG(!lspec)) { + goto done; + } + link_specifier_t *lspec_dup = link_specifier_dup(lspec); + if (BUG(!lspec_dup)) { + goto done; + } + smartlist_add(data->link_specifiers, lspec_dup); } /* Success. */ diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index e3873d2f18..253c24d643 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -565,81 +565,6 @@ retry_service_rendezvous_point(const origin_circuit_t *circ) return; } -/* Add all possible link specifiers in node to lspecs: - * - legacy ID is mandatory thus MUST be present in node; - * - include ed25519 link specifier if present in the node, and the node - * supports ed25519 link authentication, even if its link versions are not - * compatible with us; - * - include IPv4 link specifier, if the primary address is not IPv4, log a - * BUG() warning, and return an empty smartlist; - * - include IPv6 link specifier if present in the node. */ -static void -get_lspecs_from_node(const node_t *node, smartlist_t *lspecs) -{ - link_specifier_t *ls; - tor_addr_port_t ap; - - tor_assert(node); - tor_assert(lspecs); - - /* Get the relay's IPv4 address. */ - node_get_prim_orport(node, &ap); - - /* We expect the node's primary address to be a valid IPv4 address. - * This conforms to the protocol, which requires either an IPv4 or IPv6 - * address (or both). */ - if (BUG(!tor_addr_is_v4(&ap.addr)) || - BUG(!tor_addr_port_is_valid_ap(&ap, 0))) { - return; - } - - ls = link_specifier_new(); - link_specifier_set_ls_type(ls, LS_IPV4); - link_specifier_set_un_ipv4_addr(ls, tor_addr_to_ipv4h(&ap.addr)); - link_specifier_set_un_ipv4_port(ls, ap.port); - /* Four bytes IPv4 and two bytes port. */ - link_specifier_set_ls_len(ls, sizeof(ap.addr.addr.in_addr) + - sizeof(ap.port)); - smartlist_add(lspecs, ls); - - /* Legacy ID is mandatory and will always be present in node. */ - ls = link_specifier_new(); - link_specifier_set_ls_type(ls, LS_LEGACY_ID); - memcpy(link_specifier_getarray_un_legacy_id(ls), node->identity, - link_specifier_getlen_un_legacy_id(ls)); - link_specifier_set_ls_len(ls, link_specifier_getlen_un_legacy_id(ls)); - smartlist_add(lspecs, ls); - - /* ed25519 ID is only included if the node has it, and the node declares a - protocol version that supports ed25519 link authentication, even if that - link version is not compatible with us. (We are sending the ed25519 key - to another tor, which may support different link versions.) */ - if (!ed25519_public_key_is_zero(&node->ed25519_id) && - node_supports_ed25519_link_authentication(node, 0)) { - ls = link_specifier_new(); - link_specifier_set_ls_type(ls, LS_ED25519_ID); - memcpy(link_specifier_getarray_un_ed25519_id(ls), &node->ed25519_id, - link_specifier_getlen_un_ed25519_id(ls)); - link_specifier_set_ls_len(ls, link_specifier_getlen_un_ed25519_id(ls)); - smartlist_add(lspecs, ls); - } - - /* Check for IPv6. If so, include it as well. */ - if (node_has_ipv6_orport(node)) { - ls = link_specifier_new(); - node_get_pref_ipv6_orport(node, &ap); - link_specifier_set_ls_type(ls, LS_IPV6); - size_t addr_len = link_specifier_getlen_un_ipv6_addr(ls); - const uint8_t *in6_addr = tor_addr_to_in6_addr8(&ap.addr); - uint8_t *ipv6_array = link_specifier_getarray_un_ipv6_addr(ls); - memcpy(ipv6_array, in6_addr, addr_len); - link_specifier_set_un_ipv6_port(ls, ap.port); - /* Sixteen bytes IPv6 and two bytes port. */ - link_specifier_set_ls_len(ls, addr_len + sizeof(ap.port)); - smartlist_add(lspecs, ls); - } -} - /* Using the given descriptor intro point ip, the node of the * rendezvous point rp_node and the service's subcredential, populate the * already allocated intro1_data object with the needed key material and link @@ -662,10 +587,9 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip, tor_assert(subcredential); tor_assert(intro1_data); - /* Build the link specifiers from the extend information of the rendezvous - * circuit that we've picked previously. */ - rp_lspecs = smartlist_new(); - get_lspecs_from_node(rp_node, rp_lspecs); + /* Build the link specifiers from the node at the end of the rendezvous + * circuit that we opened for this introduction. */ + rp_lspecs = node_get_link_specifier_smartlist(rp_node, 0); if (smartlist_len(rp_lspecs) == 0) { /* We can't rendezvous without link specifiers. */ smartlist_free(rp_lspecs); @@ -1044,9 +968,7 @@ hs_circ_handle_introduce2(const hs_service_t *service, ret = 0; done: - SMARTLIST_FOREACH(data.link_specifiers, link_specifier_t *, lspec, - link_specifier_free(lspec)); - smartlist_free(data.link_specifiers); + link_specifier_smartlist_free(data.link_specifiers); memwipe(&data, 0, sizeof(data)); return ret; } diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 075f1d5c41..c34271efca 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -546,13 +546,15 @@ find_desc_intro_point_by_legacy_id(const char *legacy_id, SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points, hs_desc_intro_point_t *, ip) { SMARTLIST_FOREACH_BEGIN(ip->link_specifiers, - const hs_desc_link_specifier_t *, lspec) { + const link_specifier_t *, lspec) { /* Not all tor node have an ed25519 identity key so we still rely on the * legacy identity digest. */ - if (lspec->type != LS_LEGACY_ID) { + if (link_specifier_get_ls_type(lspec) != LS_LEGACY_ID) { continue; } - if (fast_memneq(legacy_id, lspec->u.legacy_id, DIGEST_LEN)) { + if (fast_memneq(legacy_id, + link_specifier_getconstarray_un_legacy_id(lspec), + DIGEST_LEN)) { break; } /* Found it. */ @@ -771,24 +773,13 @@ STATIC extend_info_t * desc_intro_point_to_extend_info(const hs_desc_intro_point_t *ip) { extend_info_t *ei; - smartlist_t *lspecs = smartlist_new(); tor_assert(ip); - /* We first encode the descriptor link specifiers into the binary - * representation which is a trunnel object. */ - SMARTLIST_FOREACH_BEGIN(ip->link_specifiers, - const hs_desc_link_specifier_t *, desc_lspec) { - link_specifier_t *lspec = hs_desc_lspec_to_trunnel(desc_lspec); - smartlist_add(lspecs, lspec); - } SMARTLIST_FOREACH_END(desc_lspec); - /* Explicitly put the direct connection option to 0 because this is client * side and there is no such thing as a non anonymous client. */ - ei = hs_get_extend_info_from_lspecs(lspecs, &ip->onion_key, 0); + ei = hs_get_extend_info_from_lspecs(ip->link_specifiers, &ip->onion_key, 0); - SMARTLIST_FOREACH(lspecs, link_specifier_t *, ls, link_specifier_free(ls)); - smartlist_free(lspecs); return ei; } diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c index 14655c53a5..b2227432d2 100644 --- a/src/feature/hs/hs_common.c +++ b/src/feature/hs/hs_common.c @@ -1010,24 +1010,6 @@ hs_build_address(const ed25519_public_key_t *key, uint8_t version, tor_assert(hs_address_is_valid(addr_out)); } -/* Return a newly allocated copy of lspec. */ -link_specifier_t * -hs_link_specifier_dup(const link_specifier_t *lspec) -{ - link_specifier_t *result = link_specifier_new(); - memcpy(result, lspec, sizeof(*result)); - /* The unrecognized field is a dynamic array so make sure to copy its - * content and not the pointer. */ - link_specifier_setlen_un_unrecognized( - result, link_specifier_getlen_un_unrecognized(lspec)); - if (link_specifier_getlen_un_unrecognized(result)) { - memcpy(link_specifier_getarray_un_unrecognized(result), - link_specifier_getconstarray_un_unrecognized(lspec), - link_specifier_getlen_un_unrecognized(result)); - } - return result; -} - /* From a given ed25519 public key pk and an optional secret, compute a * blinded public key and put it in blinded_pk_out. This is only useful to * the client side because the client only has access to the identity public @@ -1698,6 +1680,12 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, tor_assert(lspecs); + if (smartlist_len(lspecs) == 0) { + log_fn(LOG_PROTOCOL_WARN, LD_REND, "Empty link specifier list."); + /* Return NULL. */ + goto done; + } + SMARTLIST_FOREACH_BEGIN(lspecs, const link_specifier_t *, ls) { switch (link_specifier_get_ls_type(ls)) { case LS_IPV4: @@ -1731,6 +1719,12 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, /* Legacy ID is mandatory, and we require IPv4. */ if (!have_v4 || !have_legacy_id) { + bool both = !have_v4 && !have_legacy_id; + log_fn(LOG_PROTOCOL_WARN, LD_REND, "Missing %s%s%s link specifier%s.", + !have_v4 ? "IPv4" : "", + both ? " and " : "", + !have_legacy_id ? "legacy ID" : "", + both ? "s" : ""); goto done; } @@ -1749,6 +1743,10 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, * release. */ } else { /* If we can't reach IPv4, return NULL. */ + log_fn(LOG_PROTOCOL_WARN, LD_REND, + "Received an IPv4 link specifier, " + "but the address is not reachable: %s:%u", + fmt_addr(&addr_v4), port_v4); goto done; } @@ -1756,7 +1754,7 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, validate: /* We'll validate now that the address we've picked isn't a private one. If - * it is, are we allowing to extend to private address? */ + * it is, are we allowed to extend to private addresses? */ if (!extend_info_addr_is_allowed(&addr_v4)) { log_fn(LOG_PROTOCOL_WARN, LD_REND, "Requested address is private and we are not allowed to extend to " @@ -1828,3 +1826,42 @@ hs_inc_rdv_stream_counter(origin_circuit_t *circ) tor_assert_nonfatal_unreached(); } } + +/* Return a newly allocated link specifier object that is a copy of dst. */ +link_specifier_t * +link_specifier_dup(const link_specifier_t *src) +{ + link_specifier_t *dup = NULL; + uint8_t *buf = NULL; + + if (BUG(!src)) { + goto err; + } + + ssize_t encoded_len_alloc = link_specifier_encoded_len(src); + if (BUG(encoded_len_alloc < 0)) { + goto err; + } + + buf = tor_malloc_zero(encoded_len_alloc); + ssize_t encoded_len_data = link_specifier_encode(buf, + encoded_len_alloc, + src); + if (BUG(encoded_len_data < 0)) { + goto err; + } + + ssize_t parsed_len = link_specifier_parse(&dup, buf, encoded_len_alloc); + if (BUG(parsed_len < 0)) { + goto err; + } + + goto done; + + err: + dup = NULL; + + done: + tor_free(buf); + return dup; +} diff --git a/src/feature/hs/hs_common.h b/src/feature/hs/hs_common.h index a44505930a..abf39fa431 100644 --- a/src/feature/hs/hs_common.h +++ b/src/feature/hs/hs_common.h @@ -217,8 +217,6 @@ uint64_t hs_get_time_period_num(time_t now); uint64_t hs_get_next_time_period_num(time_t now); time_t hs_get_start_time_of_next_time_period(time_t now); -link_specifier_t *hs_link_specifier_dup(const link_specifier_t *lspec); - MOCK_DECL(int, hs_in_period_between_tp_and_srv, (const networkstatus_t *consensus, time_t now)); @@ -262,6 +260,8 @@ extend_info_t *hs_get_extend_info_from_lspecs(const smartlist_t *lspecs, const struct curve25519_public_key_t *onion_key, int direct_conn); +link_specifier_t *link_specifier_dup(const link_specifier_t *src); + #ifdef HS_COMMON_PRIVATE STATIC void get_disaster_srv(uint64_t time_period_num, uint8_t *srv_out); diff --git a/src/feature/hs/hs_descriptor.c b/src/feature/hs/hs_descriptor.c index b09d50e010..8f7bdf86ef 100644 --- a/src/feature/hs/hs_descriptor.c +++ b/src/feature/hs/hs_descriptor.c @@ -324,12 +324,11 @@ encode_link_specifiers(const smartlist_t *specs) link_specifier_list_set_n_spec(lslist, smartlist_len(specs)); - SMARTLIST_FOREACH_BEGIN(specs, const hs_desc_link_specifier_t *, + SMARTLIST_FOREACH_BEGIN(specs, const link_specifier_t *, spec) { - link_specifier_t *ls = hs_desc_lspec_to_trunnel(spec); - if (ls) { - link_specifier_list_add_spec(lslist, ls); - } + link_specifier_t *ls = link_specifier_dup(spec); + tor_assert(ls); + link_specifier_list_add_spec(lslist, ls); } SMARTLIST_FOREACH_END(spec); { @@ -1190,52 +1189,22 @@ decode_link_specifiers(const char *encoded) results = smartlist_new(); for (i = 0; i < link_specifier_list_getlen_spec(specs); i++) { - hs_desc_link_specifier_t *hs_spec; link_specifier_t *ls = link_specifier_list_get_spec(specs, i); - tor_assert(ls); - - hs_spec = tor_malloc_zero(sizeof(*hs_spec)); - hs_spec->type = link_specifier_get_ls_type(ls); - switch (hs_spec->type) { - case LS_IPV4: - tor_addr_from_ipv4h(&hs_spec->u.ap.addr, - link_specifier_get_un_ipv4_addr(ls)); - hs_spec->u.ap.port = link_specifier_get_un_ipv4_port(ls); - break; - case LS_IPV6: - tor_addr_from_ipv6_bytes(&hs_spec->u.ap.addr, (const char *) - link_specifier_getarray_un_ipv6_addr(ls)); - hs_spec->u.ap.port = link_specifier_get_un_ipv6_port(ls); - break; - case LS_LEGACY_ID: - /* Both are known at compile time so let's make sure they are the same - * else we can copy memory out of bound. */ - tor_assert(link_specifier_getlen_un_legacy_id(ls) == - sizeof(hs_spec->u.legacy_id)); - memcpy(hs_spec->u.legacy_id, link_specifier_getarray_un_legacy_id(ls), - sizeof(hs_spec->u.legacy_id)); - break; - case LS_ED25519_ID: - /* Both are known at compile time so let's make sure they are the same - * else we can copy memory out of bound. */ - tor_assert(link_specifier_getlen_un_ed25519_id(ls) == - sizeof(hs_spec->u.ed25519_id)); - memcpy(hs_spec->u.ed25519_id, - link_specifier_getconstarray_un_ed25519_id(ls), - sizeof(hs_spec->u.ed25519_id)); - break; - default: - tor_free(hs_spec); + if (BUG(!ls)) { goto err; } - - smartlist_add(results, hs_spec); + link_specifier_t *ls_dup = link_specifier_dup(ls); + if (BUG(!ls_dup)) { + goto err; + } + smartlist_add(results, ls_dup); } goto done; err: if (results) { - SMARTLIST_FOREACH(results, hs_desc_link_specifier_t *, s, tor_free(s)); + SMARTLIST_FOREACH(results, link_specifier_t *, s, + link_specifier_free(s)); smartlist_free(results); results = NULL; } @@ -2878,8 +2847,8 @@ hs_desc_intro_point_free_(hs_desc_intro_point_t *ip) return; } if (ip->link_specifiers) { - SMARTLIST_FOREACH(ip->link_specifiers, hs_desc_link_specifier_t *, - ls, hs_desc_link_specifier_free(ls)); + SMARTLIST_FOREACH(ip->link_specifiers, link_specifier_t *, + ls, link_specifier_free(ls)); smartlist_free(ip->link_specifiers); } tor_cert_free(ip->auth_key_cert); @@ -2972,69 +2941,6 @@ hs_desc_authorized_client_free_(hs_desc_authorized_client_t *client) tor_free(client); } -/* Free the given descriptor link specifier. */ -void -hs_desc_link_specifier_free_(hs_desc_link_specifier_t *ls) -{ - if (ls == NULL) { - return; - } - tor_free(ls); -} - -/* Return a newly allocated descriptor link specifier using the given extend - * info and requested type. Return NULL on error. */ -hs_desc_link_specifier_t * -hs_desc_link_specifier_new(const extend_info_t *info, uint8_t type) -{ - hs_desc_link_specifier_t *ls = NULL; - - tor_assert(info); - - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = type; - switch (ls->type) { - case LS_IPV4: - if (info->addr.family != AF_INET) { - goto err; - } - tor_addr_copy(&ls->u.ap.addr, &info->addr); - ls->u.ap.port = info->port; - break; - case LS_IPV6: - if (info->addr.family != AF_INET6) { - goto err; - } - tor_addr_copy(&ls->u.ap.addr, &info->addr); - ls->u.ap.port = info->port; - break; - case LS_LEGACY_ID: - /* Bug out if the identity digest is not set */ - if (BUG(tor_mem_is_zero(info->identity_digest, - sizeof(info->identity_digest)))) { - goto err; - } - memcpy(ls->u.legacy_id, info->identity_digest, sizeof(ls->u.legacy_id)); - break; - case LS_ED25519_ID: - /* ed25519 keys are optional for intro points */ - if (ed25519_public_key_is_zero(&info->ed_identity)) { - goto err; - } - memcpy(ls->u.ed25519_id, info->ed_identity.pubkey, - sizeof(ls->u.ed25519_id)); - break; - default: - /* Unknown type is code flow error. */ - tor_assert(0); - } - - return ls; - err: - tor_free(ls); - return NULL; -} - /* From the given descriptor, remove and free every introduction point. */ void hs_descriptor_clear_intro_points(hs_descriptor_t *desc) @@ -3050,59 +2956,3 @@ hs_descriptor_clear_intro_points(hs_descriptor_t *desc) smartlist_clear(ips); } } - -/* From a descriptor link specifier object spec, returned a newly allocated - * link specifier object that is the encoded representation of spec. Return - * NULL on error. */ -link_specifier_t * -hs_desc_lspec_to_trunnel(const hs_desc_link_specifier_t *spec) -{ - tor_assert(spec); - - link_specifier_t *ls = link_specifier_new(); - link_specifier_set_ls_type(ls, spec->type); - - switch (spec->type) { - case LS_IPV4: - link_specifier_set_un_ipv4_addr(ls, - tor_addr_to_ipv4h(&spec->u.ap.addr)); - link_specifier_set_un_ipv4_port(ls, spec->u.ap.port); - /* Four bytes IPv4 and two bytes port. */ - link_specifier_set_ls_len(ls, sizeof(spec->u.ap.addr.addr.in_addr) + - sizeof(spec->u.ap.port)); - break; - case LS_IPV6: - { - size_t addr_len = link_specifier_getlen_un_ipv6_addr(ls); - const uint8_t *in6_addr = tor_addr_to_in6_addr8(&spec->u.ap.addr); - uint8_t *ipv6_array = link_specifier_getarray_un_ipv6_addr(ls); - memcpy(ipv6_array, in6_addr, addr_len); - link_specifier_set_un_ipv6_port(ls, spec->u.ap.port); - /* Sixteen bytes IPv6 and two bytes port. */ - link_specifier_set_ls_len(ls, addr_len + sizeof(spec->u.ap.port)); - break; - } - case LS_LEGACY_ID: - { - size_t legacy_id_len = link_specifier_getlen_un_legacy_id(ls); - uint8_t *legacy_id_array = link_specifier_getarray_un_legacy_id(ls); - memcpy(legacy_id_array, spec->u.legacy_id, legacy_id_len); - link_specifier_set_ls_len(ls, legacy_id_len); - break; - } - case LS_ED25519_ID: - { - size_t ed25519_id_len = link_specifier_getlen_un_ed25519_id(ls); - uint8_t *ed25519_id_array = link_specifier_getarray_un_ed25519_id(ls); - memcpy(ed25519_id_array, spec->u.ed25519_id, ed25519_id_len); - link_specifier_set_ls_len(ls, ed25519_id_len); - break; - } - default: - tor_assert_nonfatal_unreached(); - link_specifier_free(ls); - ls = NULL; - } - - return ls; -} diff --git a/src/feature/hs/hs_descriptor.h b/src/feature/hs/hs_descriptor.h index 04a8e16d63..dbe0cb1c94 100644 --- a/src/feature/hs/hs_descriptor.h +++ b/src/feature/hs/hs_descriptor.h @@ -69,28 +69,10 @@ typedef enum { HS_DESC_AUTH_ED25519 = 1 } hs_desc_auth_type_t; -/* Link specifier object that contains information on how to extend to the - * relay that is the address, port and handshake type. */ -typedef struct hs_desc_link_specifier_t { - /* Indicate the type of link specifier. See trunnel ed25519_cert - * specification. */ - uint8_t type; - - /* It must be one of these types, can't be more than one. */ - union { - /* IP address and port of the relay use to extend. */ - tor_addr_port_t ap; - /* Legacy identity. A 20-byte SHA1 identity fingerprint. */ - uint8_t legacy_id[DIGEST_LEN]; - /* ed25519 identity. A 32-byte key. */ - uint8_t ed25519_id[ED25519_PUBKEY_LEN]; - } u; -} hs_desc_link_specifier_t; - /* Introduction point information located in a descriptor. */ typedef struct hs_desc_intro_point_t { /* Link specifier(s) which details how to extend to the relay. This list - * contains hs_desc_link_specifier_t object. It MUST have at least one. */ + * contains link_specifier_t objects. It MUST have at least one. */ smartlist_t *link_specifiers; /* Onion key of the introduction point used to extend to it for the ntor @@ -261,12 +243,6 @@ void hs_desc_encrypted_data_free_(hs_desc_encrypted_data_t *desc); #define hs_desc_encrypted_data_free(desc) \ FREE_AND_NULL(hs_desc_encrypted_data_t, hs_desc_encrypted_data_free_, (desc)) -void hs_desc_link_specifier_free_(hs_desc_link_specifier_t *ls); -#define hs_desc_link_specifier_free(ls) \ - FREE_AND_NULL(hs_desc_link_specifier_t, hs_desc_link_specifier_free_, (ls)) - -hs_desc_link_specifier_t *hs_desc_link_specifier_new( - const extend_info_t *info, uint8_t type); void hs_descriptor_clear_intro_points(hs_descriptor_t *desc); MOCK_DECL(int, @@ -299,9 +275,6 @@ void hs_desc_authorized_client_free_(hs_desc_authorized_client_t *client); FREE_AND_NULL(hs_desc_authorized_client_t, \ hs_desc_authorized_client_free_, (client)) -link_specifier_t *hs_desc_lspec_to_trunnel( - const hs_desc_link_specifier_t *spec); - hs_desc_authorized_client_t *hs_desc_build_fake_authorized_client(void); void hs_desc_build_authorized_client(const uint8_t *subcredential, const curve25519_public_key_t * diff --git a/src/feature/hs/hs_intropoint.c b/src/feature/hs/hs_intropoint.c index b28a5c2b80..c9cd3a0419 100644 --- a/src/feature/hs/hs_intropoint.c +++ b/src/feature/hs/hs_intropoint.c @@ -601,8 +601,8 @@ hs_intropoint_clear(hs_intropoint_t *ip) return; } tor_cert_free(ip->auth_key_cert); - SMARTLIST_FOREACH(ip->link_specifiers, hs_desc_link_specifier_t *, ls, - hs_desc_link_specifier_free(ls)); + SMARTLIST_FOREACH(ip->link_specifiers, link_specifier_t *, ls, + link_specifier_free(ls)); smartlist_free(ip->link_specifiers); memset(ip, 0, sizeof(hs_intropoint_t)); } diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 8d286f2bad..e3d0043460 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -280,9 +280,10 @@ describe_intro_point(const hs_service_intro_point_t *ip) const char *legacy_id = NULL; SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - const hs_desc_link_specifier_t *, lspec) { - if (lspec->type == LS_LEGACY_ID) { - legacy_id = (const char *) lspec->u.legacy_id; + const link_specifier_t *, lspec) { + if (link_specifier_get_ls_type(lspec) == LS_LEGACY_ID) { + legacy_id = (const char *) + link_specifier_getconstarray_un_legacy_id(lspec); break; } } SMARTLIST_FOREACH_END(lspec); @@ -426,23 +427,16 @@ service_intro_point_free_void(void *obj) } /* Return a newly allocated service intro point and fully initialized from the - * given extend_info_t ei if non NULL. - * If is_legacy is true, we also generate the legacy key. - * If supports_ed25519_link_handshake_any is true, we add the relay's ed25519 - * key to the link specifiers. + * given node_t node, if non NULL. * - * If ei is NULL, returns a hs_service_intro_point_t with an empty link + * If node is NULL, returns a hs_service_intro_point_t with an empty link * specifier list and no onion key. (This is used for testing.) * On any other error, NULL is returned. * - * ei must be an extend_info_t containing an IPv4 address. (We will add supoort - * for IPv6 in a later release.) When calling extend_info_from_node(), pass - * 0 in for_direct_connection to make sure ei always has an IPv4 address. */ + * node must be an node_t with an IPv4 address. */ STATIC hs_service_intro_point_t * -service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy, - unsigned int supports_ed25519_link_handshake_any) +service_intro_point_new(const node_t *node) { - hs_desc_link_specifier_t *ls; hs_service_intro_point_t *ip; ip = tor_malloc_zero(sizeof(*ip)); @@ -472,12 +466,17 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy, ip->replay_cache = replaycache_new(0, 0); /* Initialize the base object. We don't need the certificate object. */ - ip->base.link_specifiers = smartlist_new(); + ip->base.link_specifiers = node_get_link_specifier_smartlist(node, 0); + + if (node == NULL) { + goto done; + } /* Generate the encryption key for this intro point. */ curve25519_keypair_generate(&ip->enc_key_kp, 0); - /* Figure out if this chosen node supports v3 or is legacy only. */ - if (is_legacy) { + /* Figure out if this chosen node supports v3 or is legacy only. + * NULL nodes are used in the unit tests. */ + if (!node_supports_ed25519_hs_intro(node)) { ip->base.is_only_legacy = 1; /* Legacy mode that is doesn't support v3+ with ed25519 auth key. */ ip->legacy_key = crypto_pk_new(); @@ -490,40 +489,9 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy, } } - if (ei == NULL) { - goto done; - } - - /* We'll try to add all link specifiers. Legacy is mandatory. - * IPv4 or IPv6 is required, and we always send IPv4. */ - ls = hs_desc_link_specifier_new(ei, LS_IPV4); - /* It is impossible to have an extend info object without a v4. */ - if (BUG(!ls)) { - goto err; - } - smartlist_add(ip->base.link_specifiers, ls); - - ls = hs_desc_link_specifier_new(ei, LS_LEGACY_ID); - /* It is impossible to have an extend info object without an identity - * digest. */ - if (BUG(!ls)) { - goto err; - } - smartlist_add(ip->base.link_specifiers, ls); - - /* ed25519 identity key is optional for intro points. If the node supports - * ed25519 link authentication, we include it. */ - if (supports_ed25519_link_handshake_any) { - ls = hs_desc_link_specifier_new(ei, LS_ED25519_ID); - if (ls) { - smartlist_add(ip->base.link_specifiers, ls); - } - } - - /* IPv6 is not supported in this release. */ - - /* Finally, copy onion key from the extend_info_t object. */ - memcpy(&ip->onion_key, &ei->curve25519_onion_key, sizeof(ip->onion_key)); + /* Finally, copy onion key from the node. */ + memcpy(&ip->onion_key, node_get_curve25519_onion_key(node), + sizeof(ip->onion_key)); done: return ip; @@ -656,16 +624,16 @@ get_objects_from_ident(const hs_ident_circuit_t *ident, * encountered in the link specifier list. Return NULL if it can't be found. * * The caller does NOT have ownership of the object, the intro point does. */ -static hs_desc_link_specifier_t * +static link_specifier_t * get_link_spec_by_type(const hs_service_intro_point_t *ip, uint8_t type) { - hs_desc_link_specifier_t *lnk_spec = NULL; + link_specifier_t *lnk_spec = NULL; tor_assert(ip); SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - hs_desc_link_specifier_t *, ls) { - if (ls->type == type) { + link_specifier_t *, ls) { + if (link_specifier_get_ls_type(ls) == type) { lnk_spec = ls; goto end; } @@ -681,7 +649,7 @@ get_link_spec_by_type(const hs_service_intro_point_t *ip, uint8_t type) STATIC const node_t * get_node_from_intro_point(const hs_service_intro_point_t *ip) { - const hs_desc_link_specifier_t *ls; + const link_specifier_t *ls; tor_assert(ip); @@ -690,7 +658,8 @@ get_node_from_intro_point(const hs_service_intro_point_t *ip) return NULL; } /* XXX In the future, we want to only use the ed25519 ID (#22173). */ - return node_get_by_id((const char *) ls->u.legacy_id); + return node_get_by_id( + (const char *) link_specifier_getconstarray_un_legacy_id(ls)); } /* Given a service intro point, return the extend_info_t for it. This can @@ -1557,7 +1526,7 @@ remember_failing_intro_point(const hs_service_intro_point_t *ip, hs_service_descriptor_t *desc, time_t now) { time_t *time_of_failure, *prev_ptr; - const hs_desc_link_specifier_t *legacy_ls; + const link_specifier_t *legacy_ls; tor_assert(ip); tor_assert(desc); @@ -1566,22 +1535,13 @@ remember_failing_intro_point(const hs_service_intro_point_t *ip, *time_of_failure = now; legacy_ls = get_link_spec_by_type(ip, LS_LEGACY_ID); tor_assert(legacy_ls); - prev_ptr = digestmap_set(desc->intro_points.failed_id, - (const char *) legacy_ls->u.legacy_id, - time_of_failure); + prev_ptr = digestmap_set( + desc->intro_points.failed_id, + (const char *) link_specifier_getconstarray_un_legacy_id(legacy_ls), + time_of_failure); tor_free(prev_ptr); } -/* Copy the descriptor link specifier object from src to dst. */ -static void -link_specifier_copy(hs_desc_link_specifier_t *dst, - const hs_desc_link_specifier_t *src) -{ - tor_assert(dst); - tor_assert(src); - memcpy(dst, src, sizeof(hs_desc_link_specifier_t)); -} - /* Using a given descriptor signing keypair signing_kp, a service intro point * object ip and the time now, setup the content of an already allocated * descriptor intro desc_ip. @@ -1616,9 +1576,14 @@ setup_desc_intro_point(const ed25519_keypair_t *signing_kp, /* Copy link specifier(s). */ SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - const hs_desc_link_specifier_t *, ls) { - hs_desc_link_specifier_t *copy = tor_malloc_zero(sizeof(*copy)); - link_specifier_copy(copy, ls); + const link_specifier_t *, ls) { + if (BUG(!ls)) { + goto done; + } + link_specifier_t *copy = link_specifier_dup(ls); + if (BUG(!copy)) { + goto done; + } smartlist_add(desc_ip->link_specifiers, copy); } SMARTLIST_FOREACH_END(ls); @@ -2107,7 +2072,6 @@ static hs_service_intro_point_t * pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) { const node_t *node; - extend_info_t *info = NULL; hs_service_intro_point_t *ip = NULL; /* Normal 3-hop introduction point flags. */ router_crn_flags_t flags = CRN_NEED_UPTIME | CRN_NEED_DESC; @@ -2128,43 +2092,17 @@ pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes) * we don't want to use that node anymore. */ smartlist_add(exclude_nodes, (void *) node); - /* We do this to ease our life but also this call makes appropriate checks - * of the node object such as validating ntor support for instance. - * - * We must provide an extend_info for clients to connect over a 3-hop path, - * so we don't pass direct_conn here. */ - info = extend_info_from_node(node, 0); - if (BUG(info == NULL)) { - goto err; - } - - /* Let's do a basic sanity check here so that we don't end up advertising the - * ed25519 identity key of relays that don't actually support the link - * protocol */ - if (!node_supports_ed25519_link_authentication(node, 0)) { - tor_assert_nonfatal(ed25519_public_key_is_zero(&info->ed_identity)); - } else { - /* Make sure we *do* have an ed key if we support the link authentication. - * Sending an empty key would result in a failure to extend. */ - tor_assert_nonfatal(!ed25519_public_key_is_zero(&info->ed_identity)); - } + /* Create our objects and populate them with the node information. */ + ip = service_intro_point_new(node); - /* Create our objects and populate them with the node information. - * We don't care if the intro's link auth is compatible with us, because - * we are sending the ed25519 key to a remote client via the descriptor. */ - ip = service_intro_point_new(info, !node_supports_ed25519_hs_intro(node), - node_supports_ed25519_link_authentication(node, - 0)); if (ip == NULL) { goto err; } - log_info(LD_REND, "Picked intro point: %s", extend_info_describe(info)); - extend_info_free(info); + log_info(LD_REND, "Picked intro point: %s", node_describe(node)); return ip; err: service_intro_point_free(ip); - extend_info_free(info); return NULL; } diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h index ec53f2f23b..8d7f773219 100644 --- a/src/feature/hs/hs_service.h +++ b/src/feature/hs/hs_service.h @@ -369,10 +369,7 @@ STATIC hs_service_t *find_service(hs_service_ht *map, STATIC void remove_service(hs_service_ht *map, hs_service_t *service); STATIC int register_service(hs_service_ht *map, hs_service_t *service); /* Service introduction point functions. */ -STATIC hs_service_intro_point_t *service_intro_point_new( - const extend_info_t *ei, - unsigned int is_legacy, - unsigned int supports_ed25519_link_handshake_any); +STATIC hs_service_intro_point_t *service_intro_point_new(const node_t *node); STATIC void service_intro_point_free_(hs_service_intro_point_t *ip); #define service_intro_point_free(ip) \ FREE_AND_NULL(hs_service_intro_point_t, \ diff --git a/src/feature/nodelist/nodelist.c b/src/feature/nodelist/nodelist.c index 9a27701803..32ee2677bb 100644 --- a/src/feature/nodelist/nodelist.c +++ b/src/feature/nodelist/nodelist.c @@ -1189,6 +1189,102 @@ node_get_rsa_id_digest(const node_t *node) return (const uint8_t*)node->identity; } +/* Returns a new smartlist with all possible link specifiers from node: + * - legacy ID is mandatory thus MUST be present in node; + * - include ed25519 link specifier if present in the node, and the node + * supports ed25519 link authentication, and: + * - if direct_conn is true, its link versions are compatible with us, + * - if direct_conn is false, regardless of its link versions; + * - include IPv4 link specifier, if the primary address is not IPv4, log a + * BUG() warning, and return an empty smartlist; + * - include IPv6 link specifier if present in the node. + * + * If node is NULL, returns an empty smartlist. + * + * The smartlist must be freed using link_specifier_smartlist_free(). */ +smartlist_t * +node_get_link_specifier_smartlist(const node_t *node, bool direct_conn) +{ + link_specifier_t *ls; + tor_addr_port_t ap; + smartlist_t *lspecs = smartlist_new(); + + if (!node) + return lspecs; + + /* Get the relay's IPv4 address. */ + node_get_prim_orport(node, &ap); + + /* We expect the node's primary address to be a valid IPv4 address. + * This conforms to the protocol, which requires either an IPv4 or IPv6 + * address (or both). */ + if (BUG(!tor_addr_is_v4(&ap.addr)) || + BUG(!tor_addr_port_is_valid_ap(&ap, 0))) { + return lspecs; + } + + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_IPV4); + link_specifier_set_un_ipv4_addr(ls, tor_addr_to_ipv4h(&ap.addr)); + link_specifier_set_un_ipv4_port(ls, ap.port); + /* Four bytes IPv4 and two bytes port. */ + link_specifier_set_ls_len(ls, sizeof(ap.addr.addr.in_addr) + + sizeof(ap.port)); + smartlist_add(lspecs, ls); + + /* Legacy ID is mandatory and will always be present in node. */ + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_LEGACY_ID); + memcpy(link_specifier_getarray_un_legacy_id(ls), node->identity, + link_specifier_getlen_un_legacy_id(ls)); + link_specifier_set_ls_len(ls, link_specifier_getlen_un_legacy_id(ls)); + smartlist_add(lspecs, ls); + + /* ed25519 ID is only included if the node has it, and the node declares a + protocol version that supports ed25519 link authentication. + If direct_conn is true, we also require that the node's link version is + compatible with us. (Otherwise, we will be sending the ed25519 key + to another tor, which may support different link versions.) */ + if (!ed25519_public_key_is_zero(&node->ed25519_id) && + node_supports_ed25519_link_authentication(node, direct_conn)) { + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_ED25519_ID); + memcpy(link_specifier_getarray_un_ed25519_id(ls), &node->ed25519_id, + link_specifier_getlen_un_ed25519_id(ls)); + link_specifier_set_ls_len(ls, link_specifier_getlen_un_ed25519_id(ls)); + smartlist_add(lspecs, ls); + } + + /* Check for IPv6. If so, include it as well. */ + if (node_has_ipv6_orport(node)) { + ls = link_specifier_new(); + node_get_pref_ipv6_orport(node, &ap); + link_specifier_set_ls_type(ls, LS_IPV6); + size_t addr_len = link_specifier_getlen_un_ipv6_addr(ls); + const uint8_t *in6_addr = tor_addr_to_in6_addr8(&ap.addr); + uint8_t *ipv6_array = link_specifier_getarray_un_ipv6_addr(ls); + memcpy(ipv6_array, in6_addr, addr_len); + link_specifier_set_un_ipv6_port(ls, ap.port); + /* Sixteen bytes IPv6 and two bytes port. */ + link_specifier_set_ls_len(ls, addr_len + sizeof(ap.port)); + smartlist_add(lspecs, ls); + } + + return lspecs; +} + +/* Free a link specifier list. */ +void +link_specifier_smartlist_free_(smartlist_t *ls_list) +{ + if (!ls_list) + return; + + SMARTLIST_FOREACH(ls_list, link_specifier_t *, lspec, + link_specifier_free(lspec)); + smartlist_free(ls_list); +} + /** Return the nickname of <b>node</b>, or NULL if we can't find one. */ const char * node_get_nickname(const node_t *node) diff --git a/src/feature/nodelist/nodelist.h b/src/feature/nodelist/nodelist.h index 3420959618..a3d65347a8 100644 --- a/src/feature/nodelist/nodelist.h +++ b/src/feature/nodelist/nodelist.h @@ -77,6 +77,11 @@ int node_supports_v3_hsdir(const node_t *node); int node_supports_ed25519_hs_intro(const node_t *node); int node_supports_v3_rendezvous_point(const node_t *node); const uint8_t *node_get_rsa_id_digest(const node_t *node); +smartlist_t *node_get_link_specifier_smartlist(const node_t *node, + bool direct_conn); +void link_specifier_smartlist_free_(smartlist_t *ls_list); +#define link_specifier_smartlist_free(ls_list) \ + FREE_AND_NULL(smartlist_t, link_specifier_smartlist_free_, (ls_list)) int node_has_ipv6_addr(const node_t *node); int node_has_ipv6_orport(const node_t *node); diff --git a/src/lib/net/address.c b/src/lib/net/address.c index e1c9e1310f..214d8aa3eb 100644 --- a/src/lib/net/address.c +++ b/src/lib/net/address.c @@ -238,9 +238,18 @@ tor_addr_make_null(tor_addr_t *a, sa_family_t family) a->family = family; } -/** Return true iff <b>ip</b> is an IP reserved to localhost or local networks - * in RFC1918 or RFC4193 or RFC4291. (fec0::/10, deprecated by RFC3879, is - * also treated as internal for now.) +/** Return true iff <b>ip</b> is an IP reserved to localhost or local networks. + * + * If <b>ip</b> is in RFC1918 or RFC4193 or RFC4291, we will return true. + * (fec0::/10, deprecated by RFC3879, is also treated as internal for now + * and will return true.) + * + * If <b>ip</b> is 0.0.0.0 or 100.64.0.0/10 (RFC6598), we will act as: + * - Internal if <b>for_listening</b> is 0, as these addresses are not + * routable on the internet and we won't be publicly accessible to clients. + * - External if <b>for_listening</b> is 1, as clients could connect to us + * from the internet (in the case of 0.0.0.0) or a service provider's + * internal network (in the case of RFC6598). */ int tor_addr_is_internal_(const tor_addr_t *addr, int for_listening, @@ -288,11 +297,13 @@ tor_addr_is_internal_(const tor_addr_t *addr, int for_listening, return 0; } else if (v_family == AF_INET) { - if (for_listening && !iph4) /* special case for binding to 0.0.0.0 */ + /* special case for binding to 0.0.0.0 or 100.64/10 (RFC6598) */ + if (for_listening && (!iph4 || ((iph4 & 0xffc00000) == 0x64400000))) return 0; if (((iph4 & 0xff000000) == 0x0a000000) || /* 10/8 */ ((iph4 & 0xff000000) == 0x00000000) || /* 0/8 */ ((iph4 & 0xff000000) == 0x7f000000) || /* 127/8 */ + ((iph4 & 0xffc00000) == 0x64400000) || /* 100.64/10 */ ((iph4 & 0xffff0000) == 0xa9fe0000) || /* 169.254/16 */ ((iph4 & 0xfff00000) == 0xac100000) || /* 172.16/12 */ ((iph4 & 0xffff0000) == 0xc0a80000)) /* 192.168/16 */ diff --git a/src/lib/time/compat_time.c b/src/lib/time/compat_time.c index 33e077a587..c6625c7806 100644 --- a/src/lib/time/compat_time.c +++ b/src/lib/time/compat_time.c @@ -522,7 +522,9 @@ monotime_init_internal(void) GetTickCount64_fn = (GetTickCount64_fn_t) GetProcAddress(h, "GetTickCount64"); } - // FreeLibrary(h) ? + // We can't call FreeLibrary(h) here, because freeing the handle may + // unload the library, and cause future calls to GetTickCount64_fn() + // to fail. See 29642 for details. } void diff --git a/src/test/hs_test_helpers.c b/src/test/hs_test_helpers.c index f2ae8398df..c57bdc730b 100644 --- a/src/test/hs_test_helpers.c +++ b/src/test/hs_test_helpers.c @@ -21,26 +21,35 @@ hs_helper_build_intro_point(const ed25519_keypair_t *signing_kp, time_t now, /* For a usable intro point we need at least two link specifiers: One legacy * keyid and one ipv4 */ { - hs_desc_link_specifier_t *ls_legacy = tor_malloc_zero(sizeof(*ls_legacy)); - hs_desc_link_specifier_t *ls_v4 = tor_malloc_zero(sizeof(*ls_v4)); - ls_legacy->type = LS_LEGACY_ID; - memcpy(ls_legacy->u.legacy_id, "0299F268FCA9D55CD157976D39AE92B4B455B3A8", - DIGEST_LEN); - ls_v4->u.ap.port = 9001; - int family = tor_addr_parse(&ls_v4->u.ap.addr, addr); + tor_addr_t a; + tor_addr_make_unspec(&a); + link_specifier_t *ls_legacy = link_specifier_new(); + link_specifier_t *ls_ip = link_specifier_new(); + link_specifier_set_ls_type(ls_legacy, LS_LEGACY_ID); + memset(link_specifier_getarray_un_legacy_id(ls_legacy), 'C', + link_specifier_getlen_un_legacy_id(ls_legacy)); + int family = tor_addr_parse(&a, addr); switch (family) { case AF_INET: - ls_v4->type = LS_IPV4; + link_specifier_set_ls_type(ls_ip, LS_IPV4); + link_specifier_set_un_ipv4_addr(ls_ip, tor_addr_to_ipv4h(&a)); + link_specifier_set_un_ipv4_port(ls_ip, 9001); break; case AF_INET6: - ls_v4->type = LS_IPV6; + link_specifier_set_ls_type(ls_ip, LS_IPV6); + memcpy(link_specifier_getarray_un_ipv6_addr(ls_ip), + tor_addr_to_in6_addr8(&a), + link_specifier_getlen_un_ipv6_addr(ls_ip)); + link_specifier_set_un_ipv6_port(ls_ip, 9001); break; default: - /* Stop the test, not suppose to have an error. */ - tt_int_op(family, OP_EQ, AF_INET); + /* Stop the test, not supposed to have an error. + * Compare with -1 to show the actual family. + */ + tt_int_op(family, OP_EQ, -1); } smartlist_add(ip->link_specifiers, ls_legacy); - smartlist_add(ip->link_specifiers, ls_v4); + smartlist_add(ip->link_specifiers, ls_ip); } ret = ed25519_keypair_generate(&auth_kp, 0); @@ -153,8 +162,11 @@ hs_helper_build_hs_desc_impl(unsigned int no_ip, /* Add four intro points. */ smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "1.2.3.4", 0)); +/* IPv6-only introduction points are not supported yet, see #23588 */ +#if 0 smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "[2600::1]", 0)); +#endif smartlist_add(desc->encrypted_data.intro_points, hs_helper_build_intro_point(signing_kp, now, "3.2.1.4", 1)); smartlist_add(desc->encrypted_data.intro_points, @@ -202,7 +214,6 @@ void hs_helper_desc_equal(const hs_descriptor_t *desc1, const hs_descriptor_t *desc2) { - char *addr1 = NULL, *addr2 = NULL; /* Plaintext data section. */ tt_int_op(desc1->plaintext_data.version, OP_EQ, desc2->plaintext_data.version); @@ -291,35 +302,57 @@ hs_helper_desc_equal(const hs_descriptor_t *desc1, tt_int_op(smartlist_len(ip1->link_specifiers), ==, smartlist_len(ip2->link_specifiers)); for (int j = 0; j < smartlist_len(ip1->link_specifiers); j++) { - hs_desc_link_specifier_t *ls1 = smartlist_get(ip1->link_specifiers, j), - *ls2 = smartlist_get(ip2->link_specifiers, j); - tt_int_op(ls1->type, ==, ls2->type); - switch (ls1->type) { + link_specifier_t *ls1 = smartlist_get(ip1->link_specifiers, j), + *ls2 = smartlist_get(ip2->link_specifiers, j); + tt_int_op(link_specifier_get_ls_type(ls1), ==, + link_specifier_get_ls_type(ls2)); + switch (link_specifier_get_ls_type(ls1)) { case LS_IPV4: + { + uint32_t addr1 = link_specifier_get_un_ipv4_addr(ls1); + uint32_t addr2 = link_specifier_get_un_ipv4_addr(ls2); + tt_int_op(addr1, OP_EQ, addr2); + uint16_t port1 = link_specifier_get_un_ipv4_port(ls1); + uint16_t port2 = link_specifier_get_un_ipv4_port(ls2); + tt_int_op(port1, ==, port2); + } + break; case LS_IPV6: { - addr1 = tor_addr_to_str_dup(&ls1->u.ap.addr); - addr2 = tor_addr_to_str_dup(&ls2->u.ap.addr); - tt_str_op(addr1, OP_EQ, addr2); - tor_free(addr1); - tor_free(addr2); - tt_int_op(ls1->u.ap.port, ==, ls2->u.ap.port); + const uint8_t *addr1 = + link_specifier_getconstarray_un_ipv6_addr(ls1); + const uint8_t *addr2 = + link_specifier_getconstarray_un_ipv6_addr(ls2); + tt_int_op(link_specifier_getlen_un_ipv6_addr(ls1), OP_EQ, + link_specifier_getlen_un_ipv6_addr(ls2)); + tt_mem_op(addr1, OP_EQ, addr2, + link_specifier_getlen_un_ipv6_addr(ls1)); + uint16_t port1 = link_specifier_get_un_ipv6_port(ls1); + uint16_t port2 = link_specifier_get_un_ipv6_port(ls2); + tt_int_op(port1, ==, port2); } break; case LS_LEGACY_ID: - tt_mem_op(ls1->u.legacy_id, OP_EQ, ls2->u.legacy_id, - sizeof(ls1->u.legacy_id)); + { + const uint8_t *id1 = + link_specifier_getconstarray_un_legacy_id(ls1); + const uint8_t *id2 = + link_specifier_getconstarray_un_legacy_id(ls2); + tt_int_op(link_specifier_getlen_un_legacy_id(ls1), OP_EQ, + link_specifier_getlen_un_legacy_id(ls2)); + tt_mem_op(id1, OP_EQ, id2, + link_specifier_getlen_un_legacy_id(ls1)); + } break; default: /* Unknown type, caught it and print its value. */ - tt_int_op(ls1->type, OP_EQ, -1); + tt_int_op(link_specifier_get_ls_type(ls1), OP_EQ, -1); } } } } done: - tor_free(addr1); - tor_free(addr2); + ; } diff --git a/src/test/test-network.sh b/src/test/test-network.sh index 372c8cbac3..4d56e83806 100755 --- a/src/test/test-network.sh +++ b/src/test/test-network.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash # This script calls the equivalent script in chutney/tools @@ -18,6 +18,10 @@ ECHO="${ECHO:-echo}" # Output is prefixed with the name of the script myname=$(basename "$0") +# Save the arguments before we destroy them +# This might not preserve arguments with spaces in them +ORIGINAL_ARGS=( "$@" ) + # We need to find CHUTNEY_PATH, so that we can call the version of this script # in chutney/tools with the same arguments. We also need to respect --quiet. until [ -z "$1" ] @@ -95,7 +99,7 @@ if [ -d "$CHUTNEY_PATH" ] && [ -x "$TEST_NETWORK" ]; then # this may fail if some arguments have spaces in them # if so, set CHUTNEY_PATH before calling test-network.sh, and spaces # will be handled correctly - exec "$TEST_NETWORK" "$@" + exec "$TEST_NETWORK" "${ORIGINAL_ARGS[@]}" # $ORIGINAL_ARGS else $ECHO "$myname: Could not find tools/test-network.sh in CHUTNEY_PATH." $ECHO "$myname: Please update your chutney using 'git pull'." diff --git a/src/test/test_addr.c b/src/test/test_addr.c index 1aa7a3dcd6..fb8df5f0fb 100644 --- a/src/test/test_addr.c +++ b/src/test/test_addr.c @@ -1243,6 +1243,23 @@ test_addr_make_null(void *data) tor_free(zeros); } +#define TEST_ADDR_INTERNAL(a, for_listening, rv) STMT_BEGIN \ + tor_addr_t t; \ + tt_int_op(tor_inet_pton(AF_INET, a, &t.addr.in_addr), OP_EQ, 1); \ + t.family = AF_INET; \ + tt_int_op(tor_addr_is_internal(&t, for_listening), OP_EQ, rv); \ + STMT_END; + +static void +test_addr_rfc6598(void *arg) +{ + (void)arg; + TEST_ADDR_INTERNAL("100.64.0.1", 0, 1); + TEST_ADDR_INTERNAL("100.64.0.1", 1, 0); + done: + ; +} + #define ADDR_LEGACY(name) \ { #name, test_addr_ ## name , 0, NULL, NULL } @@ -1257,5 +1274,6 @@ struct testcase_t addr_tests[] = { { "sockaddr_to_str", test_addr_sockaddr_to_str, 0, NULL, NULL }, { "is_loopback", test_addr_is_loopback, 0, NULL, NULL }, { "make_null", test_addr_make_null, 0, NULL, NULL }, + { "rfc6598", test_addr_rfc6598, 0, NULL, NULL }, END_OF_TESTCASES }; diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index eee1edc50c..1976e54bb9 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -333,7 +333,7 @@ test_circuitpadding_rtt(void *arg) OP_EQ, relay_side->padding_info[0]->rtt_estimate_usec+ circpad_machine_current_state( - relay_side->padding_info[0])->start_usec); + relay_side->padding_info[0])->histogram_edges[0]); circpad_cell_event_nonpadding_received((circuit_t*)relay_side); circpad_cell_event_nonpadding_received((circuit_t*)relay_side); @@ -349,7 +349,7 @@ test_circuitpadding_rtt(void *arg) OP_EQ, relay_side->padding_info[0]->rtt_estimate_usec+ circpad_machine_current_state( - relay_side->padding_info[0])->start_usec); + relay_side->padding_info[0])->histogram_edges[0]); /* Test 2: Termination of RTT measurement (from the previous test) */ tt_int_op(relay_side->padding_info[0]->stop_rtt_update, OP_EQ, 1); @@ -367,7 +367,7 @@ test_circuitpadding_rtt(void *arg) OP_EQ, relay_side->padding_info[0]->rtt_estimate_usec+ circpad_machine_current_state( - relay_side->padding_info[0])->start_usec); + relay_side->padding_info[0])->histogram_edges[0]); /* Test 3: Make sure client side machine properly ignores RTT */ circpad_cell_event_nonpadding_received((circuit_t*)client_side); @@ -383,7 +383,7 @@ test_circuitpadding_rtt(void *arg) tt_int_op(circpad_histogram_bin_to_usec(client_side->padding_info[0], 0), OP_EQ, circpad_machine_current_state( - client_side->padding_info[0])->start_usec); + client_side->padding_info[0])->histogram_edges[0]); done: free_fake_orcirc(relay_side); circuitmux_detach_all_circuits(dummy_channel.cmux, NULL); @@ -414,19 +414,23 @@ helper_create_basic_machine(void) circ_client_machine.states[CIRCPAD_STATE_BURST]. next_state[CIRCPAD_EVENT_NONPADDING_SENT] = CIRCPAD_STATE_CANCEL; - // FIXME: Is this what we want? circ_client_machine.states[CIRCPAD_STATE_BURST].token_removal = CIRCPAD_TOKEN_REMOVAL_HIGHER; - // FIXME: Tune this histogram circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_len = 5; - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 500; - circ_client_machine.states[CIRCPAD_STATE_BURST].range_usec = 1000000; + + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 500; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[1] = 2500; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 5000; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[3] = 10000; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[4] = 20000; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram[0] = 1; circ_client_machine.states[CIRCPAD_STATE_BURST].histogram[1] = 0; circ_client_machine.states[CIRCPAD_STATE_BURST].histogram[2] = 2; circ_client_machine.states[CIRCPAD_STATE_BURST].histogram[3] = 2; circ_client_machine.states[CIRCPAD_STATE_BURST].histogram[4] = 2; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_total_tokens = 7; circ_client_machine.states[CIRCPAD_STATE_BURST].use_rtt_estimate = 1; @@ -458,15 +462,25 @@ helper_create_machine_with_big_histogram(circpad_removal_t removal_strategy) burst_state->token_removal = CIRCPAD_TOKEN_REMOVAL_HIGHER; burst_state->histogram_len = BIG_HISTOGRAM_LEN; - burst_state->start_usec = 0; - burst_state->range_usec = 1000; int n_tokens = 0; - for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + int i; + for (i = 0; i < BIG_HISTOGRAM_LEN ; i++) { burst_state->histogram[i] = tokens_per_bin; n_tokens += tokens_per_bin; } + burst_state->histogram_edges[0] = 0; + burst_state->histogram_edges[1] = 1; + burst_state->histogram_edges[2] = 7; + burst_state->histogram_edges[3] = 15; + burst_state->histogram_edges[4] = 31; + burst_state->histogram_edges[5] = 62; + burst_state->histogram_edges[6] = 125; + burst_state->histogram_edges[7] = 250; + burst_state->histogram_edges[8] = 500; + burst_state->histogram_edges[9] = 1000; + burst_state->histogram_total_tokens = n_tokens; burst_state->length_dist.type = CIRCPAD_DIST_UNIFORM; burst_state->length_dist.param1 = n_tokens; @@ -527,12 +541,20 @@ test_circuitpadding_token_removal_higher(void *arg) /* Test left boundaries of each histogram bin: */ const circpad_delay_t bin_left_bounds[] = - {0, 1, 7, 15, 31, 62, 125, 250, 500, CIRCPAD_DELAY_INFINITE}; - for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + {0, 1, 7, 15, 31, 62, 125, 250, 500, 1000, CIRCPAD_DELAY_INFINITE}; + for (int i = 0; i <= BIG_HISTOGRAM_LEN ; i++) { tt_uint_op(bin_left_bounds[i], OP_EQ, circpad_histogram_bin_to_usec(mi, i)); } + /* Test right boundaries of each histogram bin: */ + const circpad_delay_t bin_right_bounds[] = + {0, 6, 14, 30, 61, 124, 249, 499, 999, CIRCPAD_DELAY_INFINITE-1}; + for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + tt_uint_op(bin_right_bounds[i], OP_EQ, + histogram_get_bin_upper_bound(mi, i)); + } + /* Check that all bins have two tokens right now */ for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { tt_int_op(mi->histogram[i], OP_EQ, 2); @@ -576,8 +598,8 @@ test_circuitpadding_token_removal_higher(void *arg) /* Test below the lowest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; - mi->padding_scheduled_at_usec = current_time - 1; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100; + mi->padding_scheduled_at_usec = current_time; circpad_machine_remove_token(mi); tt_int_op(mi->histogram[0], OP_EQ, 1); @@ -624,8 +646,8 @@ test_circuitpadding_token_removal_lower(void *arg) /* Test left boundaries of each histogram bin: */ const circpad_delay_t bin_left_bounds[] = - {0, 1, 7, 15, 31, 62, 125, 250, 500, CIRCPAD_DELAY_INFINITE}; - for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + {0, 1, 7, 15, 31, 62, 125, 250, 500, 1000, CIRCPAD_DELAY_INFINITE}; + for (int i = 0; i <= BIG_HISTOGRAM_LEN ; i++) { tt_uint_op(bin_left_bounds[i], OP_EQ, circpad_histogram_bin_to_usec(mi, i)); } @@ -673,7 +695,8 @@ test_circuitpadding_token_removal_lower(void *arg) /* Test above the highest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST]. + histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; circpad_machine_remove_token(mi); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); @@ -721,8 +744,8 @@ test_circuitpadding_closest_token_removal(void *arg) /* Test left boundaries of each histogram bin: */ const circpad_delay_t bin_left_bounds[] = - {0, 1, 7, 15, 31, 62, 125, 250, 500, CIRCPAD_DELAY_INFINITE}; - for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + {0, 1, 7, 15, 31, 62, 125, 250, 500, 1000, CIRCPAD_DELAY_INFINITE}; + for (int i = 0; i <= BIG_HISTOGRAM_LEN ; i++) { tt_uint_op(bin_left_bounds[i], OP_EQ, circpad_histogram_bin_to_usec(mi, i)); } @@ -769,7 +792,9 @@ test_circuitpadding_closest_token_removal(void *arg) /* Test below the lowest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[1] = 101; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; circpad_machine_remove_token(mi); @@ -778,7 +803,6 @@ test_circuitpadding_closest_token_removal(void *arg) /* Test above the highest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; mi->padding_scheduled_at_usec = current_time - 29202; circpad_machine_remove_token(mi); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); @@ -826,8 +850,8 @@ test_circuitpadding_closest_token_removal_usec(void *arg) /* Test left boundaries of each histogram bin: */ const circpad_delay_t bin_left_bounds[] = - {0, 1, 7, 15, 31, 62, 125, 250, 500, CIRCPAD_DELAY_INFINITE}; - for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { + {0, 1, 7, 15, 31, 62, 125, 250, 500, 1000, CIRCPAD_DELAY_INFINITE}; + for (int i = 0; i <= BIG_HISTOGRAM_LEN ; i++) { tt_uint_op(bin_left_bounds[i], OP_EQ, circpad_histogram_bin_to_usec(mi, i)); } @@ -877,7 +901,9 @@ test_circuitpadding_closest_token_removal_usec(void *arg) /* Test below the lowest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[1] = 101; + circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; circpad_machine_remove_token(mi); @@ -886,7 +912,8 @@ test_circuitpadding_closest_token_removal_usec(void *arg) /* Test above the highest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); - circ_client_machine.states[CIRCPAD_STATE_BURST].start_usec = 100; + circ_client_machine.states[CIRCPAD_STATE_BURST]. + histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; circpad_machine_remove_token(mi); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); @@ -1043,7 +1070,7 @@ test_circuitpadding_tokens(void *arg) // Test 1: converting usec->bin->usec->bin // Bin 0+1 have different semantics. - for (circpad_delay_t i = 0; i <= state->start_usec+1; i++) { + for (circpad_delay_t i = 0; i <= state->histogram_edges[0]; i++) { int bin = circpad_histogram_usec_to_bin(client_side->padding_info[0], i); circpad_delay_t usec = @@ -1053,8 +1080,9 @@ test_circuitpadding_tokens(void *arg) tt_int_op(bin, OP_EQ, bin2); tt_int_op(i, OP_LE, usec); } - for (circpad_delay_t i = state->start_usec+1; - i <= state->start_usec + state->range_usec; i++) { + for (circpad_delay_t i = state->histogram_edges[0]+1; + i <= state->histogram_edges[0] + + state->histogram_edges[state->histogram_len-2]; i++) { int bin = circpad_histogram_usec_to_bin(client_side->padding_info[0], i); circpad_delay_t usec = @@ -1106,8 +1134,7 @@ test_circuitpadding_tokens(void *arg) /* 2.c. Bin 0 */ { tt_int_op(mi->histogram[0], OP_EQ, 1); - circpad_machine_remove_higher_token(mi, - state->start_usec/2); + circpad_machine_remove_higher_token(mi, state->histogram_edges[0]/2); tt_int_op(mi->histogram[0], OP_EQ, 0); } @@ -1126,8 +1153,7 @@ test_circuitpadding_tokens(void *arg) /* 3.a. Bin 0 */ { tt_int_op(mi->histogram[0], OP_EQ, 1); - circpad_machine_remove_higher_token(mi, - state->start_usec/2); + circpad_machine_remove_higher_token(mi, state->histogram_edges[0]/2); tt_int_op(mi->histogram[0], OP_EQ, 0); } @@ -1615,15 +1641,20 @@ helper_create_conditional_machine(void) ret->states[CIRCPAD_STATE_BURST]. next_state[CIRCPAD_EVENT_LENGTH_COUNT] = CIRCPAD_STATE_END; + /* Use EXACT removal strategy, otherwise setup_tokens() does not work */ ret->states[CIRCPAD_STATE_BURST].token_removal = - CIRCPAD_TOKEN_REMOVAL_NONE; + CIRCPAD_TOKEN_REMOVAL_EXACT; ret->states[CIRCPAD_STATE_BURST].histogram_len = 3; - ret->states[CIRCPAD_STATE_BURST].start_usec = 0; - ret->states[CIRCPAD_STATE_BURST].range_usec = 1000000; + + ret->states[CIRCPAD_STATE_BURST].histogram_edges[0] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram_edges[1] = 1; + ret->states[CIRCPAD_STATE_BURST].histogram_edges[2] = 1000000; + ret->states[CIRCPAD_STATE_BURST].histogram[0] = 6; ret->states[CIRCPAD_STATE_BURST].histogram[1] = 0; - ret->states[CIRCPAD_STATE_BURST].histogram[1] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram[2] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram_total_tokens = 6; ret->states[CIRCPAD_STATE_BURST].use_rtt_estimate = 0; ret->states[CIRCPAD_STATE_BURST].length_includes_nonpadding = 1; @@ -1654,8 +1685,7 @@ helper_create_conditional_machines(void) add->conditions.state_mask = CIRCPAD_CIRC_BUILDING| CIRCPAD_CIRC_NO_STREAMS|CIRCPAD_CIRC_HAS_RELAY_EARLY; add->conditions.purpose_mask = CIRCPAD_PURPOSE_ALL; - - smartlist_add(origin_padding_machines, add); + register_padding_machine(add, origin_padding_machines); add = helper_create_conditional_machine(); add->machine_num = 3; @@ -1674,15 +1704,15 @@ helper_create_conditional_machines(void) add->conditions.state_mask = CIRCPAD_CIRC_OPENED| CIRCPAD_CIRC_STREAMS|CIRCPAD_CIRC_HAS_NO_RELAY_EARLY; add->conditions.purpose_mask = CIRCPAD_PURPOSE_ALL; - smartlist_add(origin_padding_machines, add); + register_padding_machine(add, origin_padding_machines); add = helper_create_conditional_machine(); add->machine_num = 2; - smartlist_add(relay_padding_machines, add); + register_padding_machine(add, relay_padding_machines); add = helper_create_conditional_machine(); add->machine_num = 3; - smartlist_add(relay_padding_machines, add); + register_padding_machine(add, relay_padding_machines); } void @@ -2069,48 +2099,48 @@ helper_circpad_circ_distribution_machine_setup(int min, int max) zero_st->iat_dist.type = CIRCPAD_DIST_UNIFORM; zero_st->iat_dist.param1 = min; zero_st->iat_dist.param2 = max; - zero_st->start_usec = min; - zero_st->range_usec = max; + zero_st->dist_added_shift_usec = min; + zero_st->dist_max_sample_usec = max; circpad_state_t *first_st = &circ_client_machine.states[1]; first_st->next_state[CIRCPAD_EVENT_NONPADDING_RECV] = 2; first_st->iat_dist.type = CIRCPAD_DIST_LOGISTIC; first_st->iat_dist.param1 = min; first_st->iat_dist.param2 = max; - first_st->start_usec = min; - first_st->range_usec = max; + first_st->dist_added_shift_usec = min; + first_st->dist_max_sample_usec = max; circpad_state_t *second_st = &circ_client_machine.states[2]; second_st->next_state[CIRCPAD_EVENT_NONPADDING_RECV] = 3; second_st->iat_dist.type = CIRCPAD_DIST_LOG_LOGISTIC; second_st->iat_dist.param1 = min; second_st->iat_dist.param2 = max; - second_st->start_usec = min; - second_st->range_usec = max; + second_st->dist_added_shift_usec = min; + second_st->dist_max_sample_usec = max; circpad_state_t *third_st = &circ_client_machine.states[3]; third_st->next_state[CIRCPAD_EVENT_NONPADDING_RECV] = 4; third_st->iat_dist.type = CIRCPAD_DIST_GEOMETRIC; third_st->iat_dist.param1 = min; third_st->iat_dist.param2 = max; - third_st->start_usec = min; - third_st->range_usec = max; + third_st->dist_added_shift_usec = min; + third_st->dist_max_sample_usec = max; circpad_state_t *fourth_st = &circ_client_machine.states[4]; fourth_st->next_state[CIRCPAD_EVENT_NONPADDING_RECV] = 5; fourth_st->iat_dist.type = CIRCPAD_DIST_WEIBULL; fourth_st->iat_dist.param1 = min; fourth_st->iat_dist.param2 = max; - fourth_st->start_usec = min; - fourth_st->range_usec = max; + fourth_st->dist_added_shift_usec = min; + fourth_st->dist_max_sample_usec = max; circpad_state_t *fifth_st = &circ_client_machine.states[5]; fifth_st->next_state[CIRCPAD_EVENT_NONPADDING_RECV] = 6; fifth_st->iat_dist.type = CIRCPAD_DIST_PARETO; fifth_st->iat_dist.param1 = min; fifth_st->iat_dist.param2 = max; - fifth_st->start_usec = min; - fifth_st->range_usec = max; + fifth_st->dist_added_shift_usec = min; + fifth_st->dist_max_sample_usec = max; } /** Simple test that the padding delays sampled from a uniform distribution diff --git a/src/test/test_hs_cell.c b/src/test/test_hs_cell.c index 0c93f593ce..6e00e8807e 100644 --- a/src/test/test_hs_cell.c +++ b/src/test/test_hs_cell.c @@ -39,7 +39,7 @@ test_gen_establish_intro_cell(void *arg) attempt to parse it. */ { /* We only need the auth key pair here. */ - hs_service_intro_point_t *ip = service_intro_point_new(NULL, 0, 0); + hs_service_intro_point_t *ip = service_intro_point_new(NULL); /* Auth key pair is generated in the constructor so we are all set for * using this IP object. */ ret = hs_cell_build_establish_intro(circ_nonce, ip, buf); @@ -107,7 +107,7 @@ test_gen_establish_intro_cell_bad(void *arg) ed25519_sign_prefixed() function and make it fail. */ cell = trn_cell_establish_intro_new(); tt_assert(cell); - ip = service_intro_point_new(NULL, 0, 0); + ip = service_intro_point_new(NULL); cell_len = hs_cell_build_establish_intro(circ_nonce, ip, NULL); service_intro_point_free(ip); expect_log_msg_containing("Unable to make signature for " diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 2f2bb45581..8362b6cbda 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -403,6 +403,9 @@ test_client_pick_intro(void *arg) /* 2) Mark all intro points except _the chosen one_ as failed. Then query the * desc and get a random intro: check that we got _the chosen one_. */ { + /* Tell hs_get_extend_info_from_lspecs() to skip the private address check. + */ + get_options_mutable()->ExtendAllowPrivateAddresses = 1; /* Pick the chosen intro point and get its ei */ hs_desc_intro_point_t *chosen_intro_point = smartlist_get(desc->encrypted_data.intro_points, 0); @@ -476,6 +479,7 @@ test_client_pick_intro(void *arg) SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points, hs_desc_intro_point_t *, ip) { extend_info_t *intro_ei = desc_intro_point_to_extend_info(ip); + tt_assert(intro_ei); if (intro_ei) { const char *ptr; char ip_addr[TOR_ADDR_BUF_LEN]; diff --git a/src/test/test_hs_descriptor.c b/src/test/test_hs_descriptor.c index de584ed47a..09c6c3e700 100644 --- a/src/test/test_hs_descriptor.c +++ b/src/test/test_hs_descriptor.c @@ -179,115 +179,6 @@ test_descriptor_padding(void *arg) } static void -test_link_specifier(void *arg) -{ - ssize_t ret; - hs_desc_link_specifier_t spec; - smartlist_t *link_specifiers = smartlist_new(); - char buf[256]; - char *b64 = NULL; - link_specifier_t *ls = NULL; - - (void) arg; - - /* Always this port. */ - spec.u.ap.port = 42; - smartlist_add(link_specifiers, &spec); - - /* Test IPv4 for starter. */ - { - uint32_t ipv4; - - spec.type = LS_IPV4; - ret = tor_addr_parse(&spec.u.ap.addr, "1.2.3.4"); - tt_int_op(ret, OP_EQ, AF_INET); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - tt_int_op(ret, OP_EQ, 8); - /* Should be 2 bytes for port and 4 bytes for IPv4. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, 6); - ipv4 = link_specifier_get_un_ipv4_addr(ls); - tt_int_op(tor_addr_to_ipv4h(&spec.u.ap.addr), OP_EQ, ipv4); - tt_int_op(link_specifier_get_un_ipv4_port(ls), OP_EQ, spec.u.ap.port); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - /* Test IPv6. */ - { - uint8_t ipv6[16]; - - spec.type = LS_IPV6; - ret = tor_addr_parse(&spec.u.ap.addr, "[1:2:3:4::]"); - tt_int_op(ret, OP_EQ, AF_INET6); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - tt_int_op(ret, OP_EQ, 20); - /* Should be 2 bytes for port and 16 bytes for IPv6. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, 18); - for (unsigned int i = 0; i < sizeof(ipv6); i++) { - ipv6[i] = link_specifier_get_un_ipv6_addr(ls, i); - } - tt_mem_op(tor_addr_to_in6_addr8(&spec.u.ap.addr), OP_EQ, ipv6, - sizeof(ipv6)); - tt_int_op(link_specifier_get_un_ipv6_port(ls), OP_EQ, spec.u.ap.port); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - /* Test legacy. */ - { - uint8_t *id; - - spec.type = LS_LEGACY_ID; - memset(spec.u.legacy_id, 'Y', sizeof(spec.u.legacy_id)); - b64 = encode_link_specifiers(link_specifiers); - tt_assert(b64); - - /* Decode it and validate the format. */ - ret = base64_decode(buf, sizeof(buf), b64, strlen(b64)); - tt_int_op(ret, OP_GT, 0); - /* First byte is the number of link specifier. */ - tt_int_op(get_uint8(buf), OP_EQ, 1); - ret = link_specifier_parse(&ls, (uint8_t *) buf + 1, ret - 1); - /* 20 bytes digest + 1 byte type + 1 byte len. */ - tt_int_op(ret, OP_EQ, 22); - tt_int_op(link_specifier_getlen_un_legacy_id(ls), OP_EQ, DIGEST_LEN); - /* Digest length is 20 bytes. */ - tt_int_op(link_specifier_get_ls_len(ls), OP_EQ, DIGEST_LEN); - id = link_specifier_getarray_un_legacy_id(ls); - tt_mem_op(spec.u.legacy_id, OP_EQ, id, DIGEST_LEN); - - link_specifier_free(ls); - ls = NULL; - tor_free(b64); - } - - done: - link_specifier_free(ls); - tor_free(b64); - smartlist_free(link_specifiers); -} - -static void test_encode_descriptor(void *arg) { int ret; @@ -932,8 +823,6 @@ struct testcase_t hs_descriptor[] = { /* Encoding tests. */ { "cert_encoding", test_cert_encoding, TT_FORK, NULL, NULL }, - { "link_specifier", test_link_specifier, TT_FORK, - NULL, NULL }, { "encode_descriptor", test_encode_descriptor, TT_FORK, NULL, NULL }, { "descriptor_padding", test_descriptor_padding, TT_FORK, diff --git a/src/test/test_hs_intropoint.c b/src/test/test_hs_intropoint.c index 660f21ffd8..b7163c5c13 100644 --- a/src/test/test_hs_intropoint.c +++ b/src/test/test_hs_intropoint.c @@ -50,7 +50,7 @@ new_establish_intro_cell(const char *circ_nonce, /* Auth key pair is generated in the constructor so we are all set for * using this IP object. */ - ip = service_intro_point_new(NULL, 0, 0); + ip = service_intro_point_new(NULL); tt_assert(ip); cell_len = hs_cell_build_establish_intro(circ_nonce, ip, buf); tt_i64_op(cell_len, OP_GT, 0); @@ -76,7 +76,7 @@ new_establish_intro_encoded_cell(const char *circ_nonce, uint8_t *cell_out) /* Auth key pair is generated in the constructor so we are all set for * using this IP object. */ - ip = service_intro_point_new(NULL, 0, 0); + ip = service_intro_point_new(NULL); tt_assert(ip); cell_len = hs_cell_build_establish_intro(circ_nonce, ip, cell_out); tt_i64_op(cell_len, OP_GT, 0); diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 43bf894383..57132e6197 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -328,17 +328,18 @@ helper_create_service_with_clients(int num_clients) static hs_service_intro_point_t * helper_create_service_ip(void) { - hs_desc_link_specifier_t *ls; - hs_service_intro_point_t *ip = service_intro_point_new(NULL, 0, 0); + link_specifier_t *ls; + hs_service_intro_point_t *ip = service_intro_point_new(NULL); tor_assert(ip); /* Add a first unused link specifier. */ - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = LS_IPV4; + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_IPV4); smartlist_add(ip->base.link_specifiers, ls); /* Add a second link specifier used by a test. */ - ls = tor_malloc_zero(sizeof(*ls)); - ls->type = LS_LEGACY_ID; - memset(ls->u.legacy_id, 'A', sizeof(ls->u.legacy_id)); + ls = link_specifier_new(); + link_specifier_set_ls_type(ls, LS_LEGACY_ID); + memset(link_specifier_getarray_un_legacy_id(ls), 'A', + link_specifier_getlen_un_legacy_id(ls)); smartlist_add(ip->base.link_specifiers, ls); return ip; @@ -811,10 +812,11 @@ test_helper_functions(void *arg) const node_t *node = get_node_from_intro_point(ip); tt_ptr_op(node, OP_EQ, &mock_node); SMARTLIST_FOREACH_BEGIN(ip->base.link_specifiers, - hs_desc_link_specifier_t *, ls) { - if (ls->type == LS_LEGACY_ID) { + link_specifier_t *, ls) { + if (link_specifier_get_ls_type(ls) == LS_LEGACY_ID) { /* Change legacy id in link specifier which is not the mock node. */ - memset(ls->u.legacy_id, 'B', sizeof(ls->u.legacy_id)); + memset(link_specifier_getarray_un_legacy_id(ls), 'B', + link_specifier_getlen_un_legacy_id(ls)); } } SMARTLIST_FOREACH_END(ls); node = get_node_from_intro_point(ip); diff --git a/src/test/test_shared_random.c b/src/test/test_shared_random.c index 68e5498a78..8431f12b04 100644 --- a/src/test/test_shared_random.c +++ b/src/test/test_shared_random.c @@ -58,7 +58,8 @@ trusteddirserver_get_by_v3_auth_digest_m(const char *digest) } /* Setup a minimal dirauth environment by initializing the SR state and - * making sure the options are set to be an authority directory. */ + * making sure the options are set to be an authority directory. + * You must only call this function once per process. */ static void init_authority_state(void) { @@ -595,7 +596,9 @@ test_encoding(void *arg) /** Setup some SRVs in our SR state. If <b>also_current</b> is set, then set * both current and previous SRVs. - * Helper of test_vote() and test_sr_compute_srv(). */ + * Helper of test_vote() and test_sr_compute_srv(). + * You must call sr_state_free_all() to free the state at the end of each test + * function (on pass or fail). */ static void test_sr_setup_srv(int also_current) { @@ -605,6 +608,8 @@ test_sr_setup_srv(int also_current) "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ", sizeof(srv->value)); + /* sr_state_set_previous_srv() does not free() the old previous srv. */ + state_del_previous_srv(); sr_state_set_previous_srv(srv); if (also_current) { @@ -614,6 +619,8 @@ test_sr_setup_srv(int also_current) "NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN", sizeof(srv->value)); + /* sr_state_set_previous_srv() does not free() the old current srv. */ + state_del_current_srv(); sr_state_set_current_srv(srv); } } @@ -1069,12 +1076,13 @@ test_sr_get_majority_srv_from_votes(void *arg) smartlist_free(votes); } +/* Test utils that don't depend on authority state */ static void -test_utils(void *arg) +test_utils_general(void *arg) { (void) arg; - /* Testing srv_dup(). */ + /* Testing sr_srv_dup(). */ { sr_srv_t *srv = NULL, *dup_srv = NULL; const char *srv_value = @@ -1082,7 +1090,7 @@ test_utils(void *arg) srv = tor_malloc_zero(sizeof(*srv)); srv->num_reveals = 42; memcpy(srv->value, srv_value, sizeof(srv->value)); - dup_srv = srv_dup(srv); + dup_srv = sr_srv_dup(srv); tt_assert(dup_srv); tt_u64_op(dup_srv->num_reveals, OP_EQ, srv->num_reveals); tt_mem_op(dup_srv->value, OP_EQ, srv->value, sizeof(srv->value)); @@ -1133,9 +1141,19 @@ test_utils(void *arg) tt_str_op(get_phase_str(SR_PHASE_COMMIT), OP_EQ, "commit"); } + done: + return; +} + +/* Test utils that depend on authority state */ +static void +test_utils_auth(void *arg) +{ + (void)arg; + init_authority_state(); + /* Testing phase transition */ { - init_authority_state(); set_sr_phase(SR_PHASE_COMMIT); tt_int_op(is_phase_transition(SR_PHASE_REVEAL), OP_EQ, 1); tt_int_op(is_phase_transition(SR_PHASE_COMMIT), OP_EQ, 0); @@ -1146,8 +1164,193 @@ test_utils(void *arg) tt_int_op(is_phase_transition(42), OP_EQ, 1); } + /* Testing get, set, delete, clean SRVs */ + + { + /* Just set the previous SRV */ + test_sr_setup_srv(0); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Delete the SRVs one at a time */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And in the opposite order */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And both at once */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And do the gets and sets multiple times */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + state_del_previous_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + state_del_current_srv(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Now set the SRVs to NULL instead */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And in the opposite order */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And both at once */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_clean_srvs(); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + + /* And do the gets and sets multiple times */ + test_sr_setup_srv(1); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_previous_srv(NULL); + sr_state_set_previous_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + sr_state_set_current_srv(NULL); + sr_state_set_previous_srv(NULL); + sr_state_set_current_srv(NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_EQ, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); + } + + { + /* Now copy the values across */ + test_sr_setup_srv(1); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is different */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Set the current to the previous: the protocol goes the other way */ + sr_state_set_current_srv(sr_srv_dup(sr_state_get_previous_srv())); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is the same */ + tt_mem_op(sr_state_get_previous_srv(), OP_EQ, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + } + + { + /* Now copy a value onto itself */ + test_sr_setup_srv(1); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Take a copy of the old value */ + sr_srv_t old_current_srv; + memcpy(&old_current_srv, sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Check that the content is different */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Set the current to the current: the protocol never replaces an SRV with + * the same value */ + sr_state_set_current_srv(sr_srv_dup(sr_state_get_current_srv())); + /* Check that the pointers are non-NULL, and different from each other */ + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_current_srv(), OP_NE, NULL); + tt_ptr_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv()); + /* Check that the content is different between current and previous */ + tt_mem_op(sr_state_get_previous_srv(), OP_NE, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + /* Check that the content is the same as the old content */ + tt_mem_op(&old_current_srv, OP_EQ, + sr_state_get_current_srv(), sizeof(sr_srv_t)); + } + + /* I don't think we can say "expect a BUG()" in our tests. */ +#if 0 + { + /* Now copy a value onto itself without sr_srv_dup(). + * This should fail with a BUG() warning. */ + test_sr_setup_srv(1); + sr_state_set_current_srv(sr_state_get_current_srv()); + sr_state_set_previous_srv(sr_state_get_previous_srv()); + } +#endif + done: - return; + sr_state_free_all(); } static void @@ -1155,6 +1358,7 @@ test_state_transition(void *arg) { sr_state_t *state = NULL; time_t now = time(NULL); + sr_srv_t *cur = NULL; (void) arg; @@ -1193,44 +1397,47 @@ test_state_transition(void *arg) /* Test SRV rotation in our state. */ { - const sr_srv_t *cur, *prev; test_sr_setup_srv(1); - cur = sr_state_get_current_srv(); + tt_assert(sr_state_get_current_srv()); + /* Take a copy of the data, because the state owns the pointer */ + cur = sr_srv_dup(sr_state_get_current_srv()); tt_assert(cur); - /* After, current srv should be the previous and then set to NULL. */ + /* After, the previous SRV should be the same as the old current SRV, and + * the current SRV should be set to NULL */ state_rotate_srv(); - prev = sr_state_get_previous_srv(); - tt_assert(prev == cur); + tt_mem_op(sr_state_get_previous_srv(), OP_EQ, cur, sizeof(sr_srv_t)); tt_ptr_op(sr_state_get_current_srv(), OP_EQ, NULL); sr_state_clean_srvs(); + tor_free(cur); } /* New protocol run. */ { - const sr_srv_t *cur; /* Setup some new SRVs so we can confirm that a new protocol run * actually makes them rotate and compute new ones. */ test_sr_setup_srv(1); - cur = sr_state_get_current_srv(); - tt_assert(cur); + tt_assert(sr_state_get_current_srv()); + /* Take a copy of the data, because the state owns the pointer */ + cur = sr_srv_dup(sr_state_get_current_srv()); set_sr_phase(SR_PHASE_REVEAL); MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m); new_protocol_run(now); UNMOCK(get_my_v3_authority_cert); /* Rotation happened. */ - tt_assert(sr_state_get_previous_srv() == cur); + tt_mem_op(sr_state_get_previous_srv(), OP_EQ, cur, sizeof(sr_srv_t)); /* We are going into COMMIT phase so we had to rotate our SRVs. Usually * our current SRV would be NULL but a new protocol run should make us * compute a new SRV. */ tt_assert(sr_state_get_current_srv()); /* Also, make sure we did change the current. */ - tt_assert(sr_state_get_current_srv() != cur); + tt_mem_op(sr_state_get_current_srv(), OP_NE, cur, sizeof(sr_srv_t)); /* We should have our commitment alone. */ tt_int_op(digestmap_size(state->commits), OP_EQ, 1); tt_int_op(state->n_reveal_rounds, OP_EQ, 0); tt_int_op(state->n_commit_rounds, OP_EQ, 0); /* 46 here since we were at 45 just before. */ tt_u64_op(state->n_protocol_runs, OP_EQ, 46); + tor_free(cur); } /* Cleanup of SRVs. */ @@ -1241,6 +1448,7 @@ test_state_transition(void *arg) } done: + tor_free(cur); sr_state_free_all(); } @@ -1436,7 +1644,8 @@ struct testcase_t sr_tests[] = { { "sr_compute_srv", test_sr_compute_srv, TT_FORK, NULL, NULL }, { "sr_get_majority_srv_from_votes", test_sr_get_majority_srv_from_votes, TT_FORK, NULL, NULL }, - { "utils", test_utils, TT_FORK, NULL, NULL }, + { "utils_general", test_utils_general, TT_FORK, NULL, NULL }, + { "utils_auth", test_utils_auth, TT_FORK, NULL, NULL }, { "state_transition", test_state_transition, TT_FORK, NULL, NULL }, { "state_update", test_state_update, TT_FORK, NULL, NULL }, diff --git a/src/trunnel/ed25519_cert.trunnel b/src/trunnel/ed25519_cert.trunnel index 8d6483d558..e424ce5464 100644 --- a/src/trunnel/ed25519_cert.trunnel +++ b/src/trunnel/ed25519_cert.trunnel @@ -28,12 +28,6 @@ const LS_IPV6 = 0x01; const LS_LEGACY_ID = 0x02; const LS_ED25519_ID = 0x03; -// XXX hs_link_specifier_dup() violates the opaqueness of link_specifier_t by -// taking its sizeof(). If we ever want to turn on TRUNNEL_OPAQUE, or -// if we ever make link_specifier contain other types, we will -// need to refactor that function to do the copy by encoding and decoding the -// object. - // amended from tor.trunnel struct link_specifier { u8 ls_type; |