From 7bd671811ec38e8126ffc17cb922f2397c572cda Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 30 Jan 2020 09:29:07 -0500 Subject: Rewrite control_event_signal() to use signal_table. When we added the ACTIVE and DORMANT virtual signals, we taught the signal command to handle them, but we didn't teach SIGNAL event to report them. To solve this problem and prevent it from recurring, this patch revises the implementation of control_event_signal() to use the same signal_table that handle_control_signal() uses. This way, the two controller commands can't become out of sync. Fixes bug 33104; bugfix on 0.4.0.1-alpha. --- changes/bug33104 | 4 ++++ src/feature/control/control.c | 4 ++++ src/feature/control/control_events.c | 32 ++++++++++---------------------- 3 files changed, 18 insertions(+), 22 deletions(-) create mode 100644 changes/bug33104 diff --git a/changes/bug33104 b/changes/bug33104 new file mode 100644 index 0000000000..b5478df108 --- /dev/null +++ b/changes/bug33104 @@ -0,0 +1,4 @@ + o Minor bugfixes (controller): + - When receiving "ACTIVE" or "DORMANT" signals on the control port, + report them as SIGNAL events. Fixes bug 33104; bugfix on + 0.4.0.1-alpha. diff --git a/src/feature/control/control.c b/src/feature/control/control.c index 436bf423cf..71b864751f 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -158,6 +158,10 @@ control_ports_write_to_file(void) } const struct signal_name_t signal_table[] = { + /* NOTE: this table is used for handling SIGNAL commands and generating + * SIGNAL events. Order is significant: if there are two entries for the + * same numeric signal, the first one is the canonical name generated + * for the events. */ { SIGHUP, "RELOAD" }, { SIGHUP, "HUP" }, { SIGINT, "SHUTDOWN" }, diff --git a/src/feature/control/control_events.c b/src/feature/control/control_events.c index 9e0966ca54..1089b608b7 100644 --- a/src/feature/control/control_events.c +++ b/src/feature/control/control_events.c @@ -1552,29 +1552,17 @@ control_event_signal(uintptr_t signal_num) if (!control_event_is_interesting(EVENT_GOT_SIGNAL)) return 0; - switch (signal_num) { - case SIGHUP: - signal_string = "RELOAD"; + for (unsigned i = 0; signal_table[i].signal_name != NULL; ++i) { + if ((int)signal_num == signal_table[i].sig) { + signal_string = signal_table[i].signal_name; break; - case SIGUSR1: - signal_string = "DUMP"; - break; - case SIGUSR2: - signal_string = "DEBUG"; - break; - case SIGNEWNYM: - signal_string = "NEWNYM"; - break; - case SIGCLEARDNSCACHE: - signal_string = "CLEARDNSCACHE"; - break; - case SIGHEARTBEAT: - signal_string = "HEARTBEAT"; - break; - default: - log_warn(LD_BUG, "Unrecognized signal %lu in control_event_signal", - (unsigned long)signal_num); - return -1; + } + } + + if (signal_string == NULL) { + log_warn(LD_BUG, "Unrecognized signal %lu in control_event_signal", + (unsigned long)signal_num); + return -1; } send_control_event(EVENT_GOT_SIGNAL, "650 SIGNAL %s\r\n", -- cgit v1.2.3-54-g00ecf From b2c3cb1b26c41969776ed1fd1b20588430346e45 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 10 Feb 2020 11:38:36 -0500 Subject: Add tests for control_event_signal. --- src/test/test_controller_events.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/test/test_controller_events.c b/src/test/test_controller_events.c index 910aacace3..5b108ab317 100644 --- a/src/test/test_controller_events.c +++ b/src/test/test_controller_events.c @@ -16,6 +16,7 @@ #include "core/mainloop/connection.h" #include "feature/control/control_events.h" #include "test/test.h" +#include "test/log_test_helpers.h" #include "core/or/or_circuit_st.h" #include "core/or/origin_circuit_st.h" @@ -393,6 +394,43 @@ test_cntev_dirboot_defer_orconn(void *arg) UNMOCK(queue_control_event_string); } +static void +test_cntev_signal(void *arg) +{ + (void)arg; + int rv; + + MOCK(queue_control_event_string, mock_queue_control_event_string); + + /* Nothing is listening for signals, so no event should be queued. */ + rv = control_event_signal(SIGHUP); + tt_int_op(0, OP_EQ, rv); + tt_ptr_op(saved_event_str, OP_EQ, NULL); + + /* Now try with signals included in the event mask. */ + control_testing_set_global_event_mask(EVENT_MASK_(EVENT_GOT_SIGNAL)); + rv = control_event_signal(SIGHUP); + tt_int_op(0, OP_EQ, rv); + tt_str_op(saved_event_str, OP_EQ, "650 SIGNAL RELOAD\r\n"); + + rv = control_event_signal(SIGACTIVE); + tt_int_op(0, OP_EQ, rv); + tt_str_op(saved_event_str, OP_EQ, "650 SIGNAL ACTIVE\r\n"); + + /* Try a signal that doesn't exist. */ + setup_full_capture_of_logs(LOG_WARN); + tor_free(saved_event_str); + rv = control_event_signal(99999); + tt_int_op(-1, OP_EQ, rv); + tt_ptr_op(saved_event_str, OP_EQ, NULL); + expect_single_log_msg_containing("Unrecognized signal 99999"); + + done: + tor_free(saved_event_str); + teardown_capture_of_logs(); + UNMOCK(queue_control_event_string); +} + static void setup_orconn_state(orconn_event_msg_t *msg, uint64_t gid, uint64_t chan, int proxy_type) @@ -541,6 +579,7 @@ struct testcase_t controller_event_tests[] = { TEST(event_mask, TT_FORK), TEST(dirboot_defer_desc, TT_FORK), TEST(dirboot_defer_orconn, TT_FORK), + TEST(signal, TT_FORK), TEST(orconn_state, TT_FORK), TEST(orconn_state_pt, TT_FORK), TEST(orconn_state_proxy, TT_FORK), -- cgit v1.2.3-54-g00ecf