From 78cbced45cd244c8aab84e6fb8087b852769f5bc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 09:59:48 -0400 Subject: [PATCH 01/11] Rename "tell_event_loop_to_finish" to "...run_external_code" This function was never about 'finishing' the event loop, but rather about making sure that the code outside the event loop would be run at least once. --- src/or/connection_edge.c | 2 +- src/or/main.c | 4 ++-- src/or/main.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 77dac62b09..7d6667dbc9 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -999,7 +999,7 @@ connection_ap_mark_as_pending_circuit_(entry_connection_t *entry_conn, * So the fix is to tell it right now that it ought to finish its loop at * its next available opportunity. */ - tell_event_loop_to_finish(); + tell_event_loop_to_run_external_code(); } /** Mark entry_conn as no longer waiting for a circuit. */ diff --git a/src/or/main.c b/src/or/main.c index be61628341..e3a73ef03e 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -639,7 +639,7 @@ connection_should_read_from_linked_conn(connection_t *conn) * runs out of events, now we've changed our mind: tell it we want it to * finish. */ void -tell_event_loop_to_finish(void) +tell_event_loop_to_run_external_code(void) { if (!called_loop_once) { struct timeval tv = { 0, 0 }; @@ -663,7 +663,7 @@ connection_start_reading_from_linked_conn(connection_t *conn) /* make sure that the event_base_loop() function exits at * the end of its run through the current connections, so we can * activate read events for linked connections. */ - tell_event_loop_to_finish(); + tell_event_loop_to_run_external_code(); } else { tor_assert(smartlist_contains(active_linked_connection_lst, conn)); } diff --git a/src/or/main.h b/src/or/main.h index 132ab12bbb..808aa14143 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -45,7 +45,7 @@ int connection_is_writing(connection_t *conn); MOCK_DECL(void,connection_stop_writing,(connection_t *conn)); MOCK_DECL(void,connection_start_writing,(connection_t *conn)); -void tell_event_loop_to_finish(void); +void tell_event_loop_to_run_external_code(void); void connection_stop_reading_from_linked_conn(connection_t *conn); From f0c3b6238168e351835fdc64b5c93b84d6528870 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 10:16:00 -0400 Subject: [PATCH 02/11] Expose a new function to make the event loop exit once and for all. Instead of calling tor_cleanup(), exit(x), we can now call tor_shutdown_event_loop_and_exit. --- src/common/compat_libevent.h | 1 + src/or/main.c | 47 +++++++++++++++++++++++++++++++++++- src/or/main.h | 2 ++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index 834354c405..3d8672fc63 100644 --- a/src/common/compat_libevent.h +++ b/src/common/compat_libevent.h @@ -30,6 +30,7 @@ periodic_timer_t *periodic_timer_new(struct event_base *base, void periodic_timer_free(periodic_timer_t *); #define tor_event_base_loopexit event_base_loopexit +#define tor_event_base_loopbreak event_base_loopbreak /** Defines a configuration for using libevent with Tor: passed as an argument * to tor_libevent_initialize() to describe how we want to set up. */ diff --git a/src/or/main.c b/src/or/main.c index e3a73ef03e..6af61d5132 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -192,6 +192,14 @@ static smartlist_t *active_linked_connection_lst = NULL; * loop_once. If so, there's no need to trigger a loopexit in order * to handle linked connections. */ static int called_loop_once = 0; +/** Flag: if true, it's time to shut down, so the main loop should exit as + * soon as possible. + */ +static int main_loop_should_exit = 0; +/** The return value that the main loop should yield when it exits, if + * main_loop_should_exit is true. + */ +static int main_loop_exit_value = 0; /** We set this to 1 when we've opened a circuit, so we can print a log * entry to inform the user that Tor is working. We set it to 0 when @@ -648,6 +656,30 @@ tell_event_loop_to_run_external_code(void) } } +/** + * After finishing the current callback (if any), shut down the main loop, + * clean up the process, and exit with exitcode. + */ +void +tor_shutdown_event_loop_and_exit(int exitcode) +{ + if (main_loop_should_exit) + return; /* Ignore multiple calls to this function. */ + + main_loop_should_exit = 1; + main_loop_exit_value = exitcode; + + /* Unlike loopexit, loopbreak prevents other callbacks from running. */ + tor_event_base_loopbreak(tor_libevent_get_base()); +} + +/** Return true iff tor_shutdown_event_loop_and_exit() has been called. */ +int +tor_event_loop_shutdown_is_pending(void) +{ + return main_loop_should_exit; +} + /** Helper: Tell the main loop to begin reading bytes into conn from * its linked connection, if it is not doing so already. Called by * connection_start_reading and connection_start_writing as appropriate. */ @@ -2595,6 +2627,9 @@ do_main_loop(void) } #endif /* defined(HAVE_SYSTEMD) */ + main_loop_should_exit = 0; + main_loop_exit_value = 0; + return run_main_loop_until_done(); } @@ -2610,6 +2645,9 @@ run_main_loop_once(void) if (nt_service_is_stopping()) return 0; + if (main_loop_should_exit) + return 0; + #ifndef _WIN32 /* Make it easier to tell whether libevent failure is our fault or not. */ errno = 0; @@ -2656,6 +2694,9 @@ run_main_loop_once(void) } } + if (main_loop_should_exit) + return 0; + /* And here is where we put callbacks that happen "every time the event loop * runs." They must be very fast, or else the whole Tor process will get * slowed down. @@ -2684,7 +2725,11 @@ run_main_loop_until_done(void) do { loop_result = run_main_loop_once(); } while (loop_result == 1); - return loop_result; + + if (main_loop_should_exit) + return main_loop_exit_value; + else + return loop_result; } /** Libevent callback: invoked when we get a signal. diff --git a/src/or/main.h b/src/or/main.h index 808aa14143..5ab77c03ed 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -46,6 +46,8 @@ MOCK_DECL(void,connection_stop_writing,(connection_t *conn)); MOCK_DECL(void,connection_start_writing,(connection_t *conn)); void tell_event_loop_to_run_external_code(void); +void tor_shutdown_event_loop_and_exit(int exitcode); +int tor_event_loop_shutdown_is_pending(void); void connection_stop_reading_from_linked_conn(connection_t *conn); From c82cc8acb5aa43b3d96490d927dcc622f2c825e3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 10:22:04 -0400 Subject: [PATCH 03/11] Add a failsafe to kill tor if the new exit code doesn't work. It _should_ work, and I don't see a reason that it wouldn't, but just in case, add a 10 second timer to make tor die with an assertion failure if it's supposed to exit but it doesn't. --- src/or/main.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/or/main.c b/src/or/main.c index 6af61d5132..c3822dd94a 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -656,6 +656,22 @@ tell_event_loop_to_run_external_code(void) } } +/** Failsafe measure that should never actually be necessary: If + * tor_shutdown_event_loop_and_exit() somehow doesn't successfully exit the + * event loop, then this callback will kill Tor with an assertion failure + * seconds later + */ +static void +shutdown_did_not_work_callback(evutil_socket_t fd, short event, void *arg) +{ + // LCOV_EXCL_START + (void) fd; + (void) event; + (void) arg; + tor_assert_unreached(); + // LCOV_EXCL_STOP +} + /** * After finishing the current callback (if any), shut down the main loop, * clean up the process, and exit with exitcode. @@ -669,6 +685,13 @@ tor_shutdown_event_loop_and_exit(int exitcode) main_loop_should_exit = 1; main_loop_exit_value = exitcode; + /* Die with an assertion failure in ten seconds, if for some reason we don't + * exit normally. */ + struct timeval ten_seconds = { 10, 0 }; + event_base_once(tor_libevent_get_base(), -1, EV_TIMEOUT, + shutdown_did_not_work_callback, NULL, + &ten_seconds); + /* Unlike loopexit, loopbreak prevents other callbacks from running. */ tor_event_base_loopbreak(tor_libevent_get_base()); } From 9bafc3b1efd64b19695cd78fb51070ff8d3939f5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 10:25:55 -0400 Subject: [PATCH 04/11] Replace most bad exits in main.c, hibernate.c This should make most of the reasons that we hibernate cleaner. --- src/or/hibernate.c | 7 +++---- src/or/main.c | 11 +++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/or/hibernate.c b/src/or/hibernate.c index be2bc7ce95..8f665a15a4 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -818,8 +818,8 @@ hibernate_begin(hibernate_state_t new_state, time_t now) log_notice(LD_GENERAL,"SIGINT received %s; exiting now.", hibernate_state == HIBERNATE_STATE_EXITING ? "a second time" : "while hibernating"); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); + return; } if (new_state == HIBERNATE_STATE_LOWBANDWIDTH && @@ -980,8 +980,7 @@ consider_hibernation(time_t now) tor_assert(shutdown_time); if (shutdown_time <= now) { log_notice(LD_GENERAL, "Clean shutdown finished. Exiting."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); } return; /* if exiting soon, don't worry about bandwidth limits */ } diff --git a/src/or/main.c b/src/or/main.c index c3822dd94a..4d252faf20 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2776,14 +2776,13 @@ process_signal(int sig) { case SIGTERM: log_notice(LD_GENERAL,"Catching signal TERM, exiting cleanly."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); break; case SIGINT: if (!server_mode(get_options())) { /* do it now */ log_notice(LD_GENERAL,"Interrupt: exiting cleanly."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); + return; } #ifdef HAVE_SYSTEMD sd_notify(0, "STOPPING=1"); @@ -2812,8 +2811,8 @@ process_signal(int sig) #endif if (do_hup() < 0) { log_warn(LD_CONFIG,"Restart failed (config error?). Exiting."); - tor_cleanup(); - exit(1); // XXXX bad exit + tor_shutdown_event_loop_and_exit(1); + return; } #ifdef HAVE_SYSTEMD sd_notify(0, "READY=1"); From c247a2df6b7f6b5d997ef9e5ccc25eaaf3918db8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 10:27:05 -0400 Subject: [PATCH 05/11] On locking failure, return -1 instead of exit()ing. This is safe, since the only caller (options_act) will check the return value, and propagate failure. --- src/or/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/main.c b/src/or/main.c index 4d252faf20..8bfea7895e 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -3265,7 +3265,7 @@ try_locking(const or_options_t *options, int err_if_locked) r = try_locking(options, 0); if (r<0) { log_err(LD_GENERAL, "No, it's still there. Exiting."); - exit(1); // XXXX bad exit + return -1; } return r; } From 853e73e815c1be9c9e533160f803de56e7d21147 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 11:03:53 -0400 Subject: [PATCH 06/11] Return from instead of exit()ing when ed25519 key check fails. --- src/or/main.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 8bfea7895e..d47c8f4dd3 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1609,8 +1609,7 @@ check_ed_keys_callback(time_t now, const or_options_t *options) if (new_signing_key < 0 || generate_ed_link_cert(options, now, new_signing_key > 0)) { log_err(LD_OR, "Unable to update Ed25519 keys! Exiting."); - tor_cleanup(); - exit(1); // XXXX bad exit + tor_shutdown_event_loop_and_exit(1); } } return 30; From 1df43aff41eb8198712af0aa14ec2d574b26682c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 11:15:32 -0400 Subject: [PATCH 07/11] Return instead of exiting in options_init_from_torrc() --- src/or/config.c | 31 +++++++++++++++++++------------ src/or/control.c | 3 +-- src/or/main.c | 29 ++++++++++++++++++++++++----- src/or/ntmain.c | 2 +- 4 files changed, 45 insertions(+), 20 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 55ab76c813..3795aeaa39 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -4981,7 +4981,8 @@ load_torrc_from_disk(config_line_t *cmd_arg, int defaults_file) /** Read a configuration file into options, finding the configuration * file location based on the command line. After loading the file * call options_init_from_string() to load the config. - * Return 0 if success, -1 if failure. */ + * Return 0 if success, -1 if failure, and 1 if we succeeded but should exit + * anyway. */ int options_init_from_torrc(int argc, char **argv) { @@ -5008,22 +5009,22 @@ options_init_from_torrc(int argc, char **argv) if (config_line_find(cmdline_only_options, "-h") || config_line_find(cmdline_only_options, "--help")) { print_usage(); - exit(0); // XXXX bad exit, though probably harmless + return 1; } if (config_line_find(cmdline_only_options, "--list-torrc-options")) { /* For validating whether we've documented everything. */ list_torrc_options(); - exit(0); // XXXX bad exit, though probably harmless + return 1; } if (config_line_find(cmdline_only_options, "--list-deprecated-options")) { /* For validating whether what we have deprecated really exists. */ list_deprecated_options(); - exit(0); // XXXX bad exit, though probably harmless + return 1; } if (config_line_find(cmdline_only_options, "--version")) { printf("Tor version %s.\n",get_version()); - exit(0); // XXXX bad exit, though probably harmless + return 1; } if (config_line_find(cmdline_only_options, "--library-versions")) { @@ -5051,7 +5052,7 @@ options_init_from_torrc(int argc, char **argv) tor_compress_header_version_str(ZSTD_METHOD)); } //TODO: Hex versions? - exit(0); // XXXX bad exit, though probably harmless + return 1; } command = CMD_RUN_TOR; @@ -5112,7 +5113,8 @@ options_init_from_torrc(int argc, char **argv) get_options_mutable()->keygen_force_passphrase = FORCE_PASSPHRASE_OFF; } else { log_err(LD_CONFIG, "--no-passphrase specified without --keygen!"); - exit(1); // XXXX bad exit + retval = -1; + goto err; } } @@ -5121,7 +5123,8 @@ options_init_from_torrc(int argc, char **argv) get_options_mutable()->change_key_passphrase = 1; } else { log_err(LD_CONFIG, "--newpass specified without --keygen!"); - exit(1); // XXXX bad exit + retval = -1; + goto err; } } @@ -5131,17 +5134,20 @@ options_init_from_torrc(int argc, char **argv) if (fd_line) { if (get_options()->keygen_force_passphrase == FORCE_PASSPHRASE_OFF) { log_err(LD_CONFIG, "--no-passphrase specified with --passphrase-fd!"); - exit(1); // XXXX bad exit + retval = -1; + goto err; } else if (command != CMD_KEYGEN) { log_err(LD_CONFIG, "--passphrase-fd specified without --keygen!"); - exit(1); // XXXX bad exit + retval = -1; + goto err; } else { const char *v = fd_line->value; int ok = 1; long fd = tor_parse_long(v, 10, 0, INT_MAX, &ok, NULL); if (fd < 0 || ok == 0) { log_err(LD_CONFIG, "Invalid --passphrase-fd value %s", escaped(v)); - exit(1); // XXXX bad exit + retval = -1; + goto err; } get_options_mutable()->keygen_passphrase_fd = (int)fd; get_options_mutable()->use_keygen_passphrase_fd = 1; @@ -5156,7 +5162,8 @@ options_init_from_torrc(int argc, char **argv) if (key_line) { if (command != CMD_KEYGEN) { log_err(LD_CONFIG, "--master-key without --keygen!"); - exit(1); // XXXX bad exit + retval = -1; + goto err; } else { get_options_mutable()->master_key_fname = tor_strdup(key_line->value); } diff --git a/src/or/control.c b/src/or/control.c index ab164700e6..7b89a13707 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -6571,8 +6571,7 @@ monitor_owning_controller_process(const char *process_spec) "owning controller: %s. Exiting.", msg); owning_controller_process_spec = NULL; - tor_cleanup(); - exit(1); // XXXX bad exit: or questionable, at least. + tor_shutdown_event_loop_and_exit(1); } } diff --git a/src/or/main.c b/src/or/main.c index d47c8f4dd3..9121e52167 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2420,10 +2420,18 @@ do_hup(void) /* first, reload config variables, in case they've changed */ if (options->ReloadTorrcOnSIGHUP) { /* no need to provide argc/v, they've been cached in init_from_config */ - if (options_init_from_torrc(0, NULL) < 0) { + int init_rv = options_init_from_torrc(0, NULL); + if (init_rv < 0) { log_err(LD_CONFIG,"Reading config failed--see warnings above. " "For usage, try -h."); return -1; + } else if (BUG(init_rv > 0)) { + // LCOV_EXCL_START + /* This should be impossible: the only "return 1" cases in + * options_init_from_torrc are ones caused by command-line arguments; + * but they can't change while Tor is running. */ + return -1; + // LCOV_EXCL_STOP } options = get_options(); /* they have changed now */ /* Logs are only truncated the first time they are opened, but were @@ -3085,7 +3093,8 @@ activate_signal(int signal_num) } } -/** Main entry point for the Tor command-line client. +/** Main entry point for the Tor command-line client. Return 0 on "success", + * negative on "failure", and positive on "success and exit". */ int tor_init(int argc, char *argv[]) @@ -3192,9 +3201,14 @@ tor_init(int argc, char *argv[]) } atexit(exit_function); - if (options_init_from_torrc(argc,argv) < 0) { + int init_rv = options_init_from_torrc(argc,argv); + if (init_rv < 0) { log_err(LD_CONFIG,"Reading config failed--see warnings above."); return -1; + } else if (init_rv > 0) { + // We succeeded, and should exit anyway -- probably the user just said + // "--version" or something like that. + return 1; } /* The options are now initialised */ @@ -3820,8 +3834,13 @@ tor_main(int argc, char *argv[]) if (done) return result; } #endif /* defined(NT_SERVICE) */ - if (tor_init(argc, argv)<0) - return -1; + { + int init_rv = tor_init(argc, argv); + if (init_rv < 0) + return -1; + else if (init_rv > 0) + return 0; + } if (get_options()->Sandbox && get_options()->command == CMD_RUN_TOR) { sandbox_cfg_t* cfg = sandbox_init_filter(); diff --git a/src/or/ntmain.c b/src/or/ntmain.c index 9ce03e1f5a..ebbe0018bd 100644 --- a/src/or/ntmain.c +++ b/src/or/ntmain.c @@ -318,7 +318,7 @@ nt_service_main(void) printf("Service error %d : %s\n", (int) result, errmsg); tor_free(errmsg); if (result == ERROR_FAILED_SERVICE_CONTROLLER_CONNECT) { - if (tor_init(backup_argc, backup_argv) < 0) + if (tor_init(backup_argc, backup_argv)) return; switch (get_options()->command) { case CMD_RUN_TOR: From 3c99b810a46c2aba2ea4108018e78bc8f6a19013 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 11:34:03 -0400 Subject: [PATCH 08/11] Exit more carefully when options_act() fails. Also, annotate options_act() with places where we should be pre-validating values. --- src/or/config.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 3795aeaa39..6f7bf23dfd 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -835,9 +835,12 @@ set_options(or_options_t *new_val, char **msg) return -1; } if (options_act(old_options) < 0) { /* acting on the options failed. die. */ - log_err(LD_BUG, - "Acting on config options left us in a broken state. Dying."); - exit(1); // XXXX bad exit + if (! tor_event_loop_shutdown_is_pending()) { + log_err(LD_BUG, + "Acting on config options left us in a broken state. Dying."); + tor_shutdown_event_loop_and_exit(1); + } + return -1; } /* Issues a CONF_CHANGED event to notify controller of the change. If Tor is * just starting up then the old_options will be undefined. */ @@ -1664,8 +1667,11 @@ options_act(const or_options_t *old_options) return -1; } - if (consider_adding_dir_servers(options, old_options) < 0) + if (consider_adding_dir_servers(options, old_options) < 0) { + // XXXX This should get validated earlier, and committed here, to + // XXXX lower opportunities for reaching an error case. return -1; + } if (rend_non_anonymous_mode_enabled(options)) { log_warn(LD_GENERAL, "This copy of Tor was compiled or configured to run " @@ -1674,6 +1680,7 @@ options_act(const or_options_t *old_options) #ifdef ENABLE_TOR2WEB_MODE /* LCOV_EXCL_START */ + // XXXX This should move into options_validate() if (!options->Tor2webMode) { log_err(LD_CONFIG, "This copy of Tor was compiled to run in " "'tor2web mode'. It can only be run with the Tor2webMode torrc " @@ -1682,6 +1689,7 @@ options_act(const or_options_t *old_options) } /* LCOV_EXCL_STOP */ #else /* !(defined(ENABLE_TOR2WEB_MODE)) */ + // XXXX This should move into options_validate() if (options->Tor2webMode) { log_err(LD_CONFIG, "This copy of Tor was not compiled to run in " "'tor2web mode'. It cannot be run with the Tor2webMode torrc " @@ -1850,7 +1858,7 @@ options_act(const or_options_t *old_options) /* Set up accounting */ if (accounting_parse_options(options, 0)<0) { - log_warn(LD_CONFIG,"Error in accounting options"); + log_warn(LD_BUG,"Error in previously validated accounting options"); return -1; } if (accounting_is_enabled(options)) @@ -1874,6 +1882,7 @@ options_act(const or_options_t *old_options) char *http_authenticator; http_authenticator = alloc_http_authenticator(options->BridgePassword); if (!http_authenticator) { + // XXXX This should get validated in options_validate(). log_warn(LD_BUG, "Unable to allocate HTTP authenticator. Not setting " "BridgePassword."); return -1; @@ -1886,7 +1895,8 @@ options_act(const or_options_t *old_options) } if (parse_outbound_addresses(options, 0, &msg) < 0) { - log_warn(LD_BUG, "Failed parsing outbound bind addresses: %s", msg); + log_warn(LD_BUG, "Failed parsing previously validated outbound " + "bind addresses: %s", msg); tor_free(msg); return -1; } From 90daa28df403472d4a6aa7067ce6ab3264cdbe55 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 11:39:11 -0400 Subject: [PATCH 09/11] Changes file for ticket 23848 --- changes/bug23848 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changes/bug23848 diff --git a/changes/bug23848 b/changes/bug23848 new file mode 100644 index 0000000000..e2aec687ca --- /dev/null +++ b/changes/bug23848 @@ -0,0 +1,8 @@ + o Minor features (embedding): + - On most errors that would cause Tor to exit, it now tries to return + from the tor_main() function, rather than calling the system exit() + function. Most users won't notice a difference here, but it should + make a significant difference on platforms that try to run Tor inside + a separate thread: they should now be able to survive Tor's exit + conditions rather than having Tor shut down the entire process. + Closes ticket 23848. From c4a07b261b552c39b64c8d47f1ba75f4bb4f37d6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 19:10:05 -0400 Subject: [PATCH 10/11] Add unit tests for cases of starting with bogus keygen arguments In particular, this tests that we give an appropriate warning when we are told to use some keygen argument, but --keygen is not specified. --- src/test/test_keygen.sh | 109 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/test/test_keygen.sh b/src/test/test_keygen.sh index 87012cd283..b3d4d8e39a 100755 --- a/src/test/test_keygen.sh +++ b/src/test/test_keygen.sh @@ -40,6 +40,12 @@ fi CASE8=$dflt CASE9=$dflt CASE10=$dflt + CASE11A=$dflt + CASE11B=$dflt + CASE11C=$dflt + CASE11D=$dflt + CASE11E=$dflt + CASE11F=$dflt if [ $# -ge 1 ]; then eval "CASE${1}"=1 @@ -363,6 +369,109 @@ echo "==== Case 10 ok" fi +# Case 11a: -passphrase-fd without --keygen + +if [ "$CASE11A" = 1 ]; then + +ME="${DATA_DIR}/case11a" + +mkdir -p "${ME}/keys" + +${TOR} --DataDirectory "${ME}" --passphrase-fd 1 > "${ME}/stdout" && die "Successfully started with passphrase-fd but no keygen?" || true + +grep "passphrase-fd specified without --keygen" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + +echo "==== Case 11A ok" + +fi + +# Case 11b: --no-passphrase without --keygen + +if [ "$CASE11B" = 1 ]; then + +ME="${DATA_DIR}/case11b" + +mkdir -p "${ME}/keys" + +${TOR} --DataDirectory "${ME}" --no-passphrase > "${ME}/stdout" && die "Successfully started with no-passphrase but no keygen?" || true + +grep "no-passphrase specified without --keygen" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + +echo "==== Case 11B ok" + +fi + +# Case 11c: --newpass without --keygen + +if [ "$CASE11C" = 1 ]; then + +ME="${DATA_DIR}/case11C" + +mkdir -p "${ME}/keys" + +${TOR} --DataDirectory "${ME}" --newpass > "${ME}/stdout" && die "Successfully started with newpass but no keygen?" || true + +grep "newpass specified without --keygen" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + +echo "==== Case 11C ok" + +fi + +######## --master-key does not work yet, but this will test the error case +######## when it does. +# +# Case 11d: --master-key without --keygen +# +if [ "$CASE11D" = 1 ]; then +# +# ME="${DATA_DIR}/case11d" +# +# mkdir -p "${ME}/keys" +# +# ${TOR} --DataDirectory "${ME}" --master-key "${ME}/foobar" > "${ME}/stdout" && die "Successfully started with master-key but no keygen?" || true +# +# cat "${ME}/stdout" +# +# grep "master-key without --keygen" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + + echo "==== Case 11D skipped" + +fi + + +# Case 11E: Silly passphrase-fd + +if [ "$CASE11E" = 1 ]; then + +ME="${DATA_DIR}/case11E" + +mkdir -p "${ME}/keys" + +${TOR} --DataDirectory "${ME}" --keygen --passphrase-fd ewigeblumenkraft > "${ME}/stdout" && die "Successfully started with bogus passphrase-fd?" || true + +grep "Invalid --passphrase-fd value" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + +echo "==== Case 11E ok" + +fi + + +# Case 11F: --no-passphrase with --passphrase-fd + +if [ "$CASE11F" = 1 ]; then + +ME="${DATA_DIR}/case11F" + +mkdir -p "${ME}/keys" + +${TOR} --DataDirectory "${ME}" --keygen --passphrase-fd 1 --no-passphrase > "${ME}/stdout" && die "Successfully started with bogus passphrase-fd combination?" || true + +grep "no-passphrase specified with --passphrase-fd" "${ME}/stdout" >/dev/null || die "Tor didn't declare that there was a problem with the arguments." + +echo "==== Case 11F ok" + +fi + # Check cert-only. From a62f59f000f05ac5f5a7a22387007a1b8c2ee2a4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 20 Oct 2017 19:28:12 -0400 Subject: [PATCH 11/11] Mark "previously validated foo could not be set" blocks as unreachable. --- src/or/config.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/or/config.c b/src/or/config.c index 6f7bf23dfd..64e42c93ca 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1717,9 +1717,11 @@ options_act(const or_options_t *old_options) for (cl = options->Bridges; cl; cl = cl->next) { bridge_line_t *bridge_line = parse_bridge_line(cl->value); if (!bridge_line) { + // LCOV_EXCL_START log_warn(LD_BUG, "Previously validated Bridge line could not be added!"); return -1; + // LCOV_EXCL_STOP } bridge_add_from_config(bridge_line); } @@ -1727,15 +1729,19 @@ options_act(const or_options_t *old_options) } if (running_tor && hs_config_service_all(options, 0)<0) { + // LCOV_EXCL_START log_warn(LD_BUG, "Previously validated hidden services line could not be added!"); return -1; + // LCOV_EXCL_STOP } if (running_tor && rend_parse_service_authorization(options, 0) < 0) { + // LCOV_EXCL_START log_warn(LD_BUG, "Previously validated client authorization for " "hidden services could not be added!"); return -1; + // LCOV_EXCL_STOP } /* Load state */ @@ -1758,10 +1764,12 @@ options_act(const or_options_t *old_options) if (options->ClientTransportPlugin) { for (cl = options->ClientTransportPlugin; cl; cl = cl->next) { if (parse_transport_line(options, cl->value, 0, 0) < 0) { + // LCOV_EXCL_START log_warn(LD_BUG, "Previously validated ClientTransportPlugin line " "could not be added!"); return -1; + // LCOV_EXCL_STOP } } } @@ -1769,10 +1777,12 @@ options_act(const or_options_t *old_options) if (options->ServerTransportPlugin && server_mode(options)) { for (cl = options->ServerTransportPlugin; cl; cl = cl->next) { if (parse_transport_line(options, cl->value, 0, 1) < 0) { + // LCOV_EXCL_START log_warn(LD_BUG, "Previously validated ServerTransportPlugin line " "could not be added!"); return -1; + // LCOV_EXCL_STOP } } } @@ -1858,8 +1868,10 @@ options_act(const or_options_t *old_options) /* Set up accounting */ if (accounting_parse_options(options, 0)<0) { + // LCOV_EXCL_START log_warn(LD_BUG,"Error in previously validated accounting options"); return -1; + // LCOV_EXCL_STOP } if (accounting_is_enabled(options)) configure_accounting(time(NULL)); @@ -1895,10 +1907,12 @@ options_act(const or_options_t *old_options) } if (parse_outbound_addresses(options, 0, &msg) < 0) { + // LCOV_EXCL_START log_warn(LD_BUG, "Failed parsing previously validated outbound " "bind addresses: %s", msg); tor_free(msg); return -1; + // LCOV_EXCL_STOP } config_maybe_load_geoip_files_(options, old_options);