From 67d59d7d1ffa058280471154b8185b5420334458 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Feb 2020 12:53:15 -0500 Subject: [PATCH 1/8] Re-order most subsystems to correspond to dependency order. --- src/app/main/subsystem_list.c | 12 ++++++------ src/core/or/ocirc_event.c | 2 +- src/core/or/orconn_event.c | 2 +- src/feature/control/btrack.c | 2 +- src/lib/compress/compress.c | 2 +- src/lib/net/network_sys.c | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/app/main/subsystem_list.c b/src/app/main/subsystem_list.c index b4439cdc7b..6784ebe8a0 100644 --- a/src/app/main/subsystem_list.c +++ b/src/app/main/subsystem_list.c @@ -50,24 +50,24 @@ const subsys_fns_t *tor_subsystems[] = { &sys_logging, &sys_time, - &sys_network, - &sys_compress, &sys_crypto, + &sys_compress, + &sys_network, &sys_tortls, &sys_process, - &sys_orconn_event, - &sys_ocirc_event, - &sys_btrack, - &sys_evloop, &sys_mainloop, &sys_or, + &sys_orconn_event, + &sys_ocirc_event, &sys_relay, + &sys_btrack, + &sys_dirauth, }; diff --git a/src/core/or/ocirc_event.c b/src/core/or/ocirc_event.c index 66992a0b5f..6aa7f684e0 100644 --- a/src/core/or/ocirc_event.c +++ b/src/core/or/ocirc_event.c @@ -123,6 +123,6 @@ ocirc_cevent_publish(ocirc_cevent_msg_t *msg) const subsys_fns_t sys_ocirc_event = { .name = "ocirc_event", .supported = true, - .level = -32, + .level = 22, .add_pubsub = ocirc_add_pubsub, }; diff --git a/src/core/or/orconn_event.c b/src/core/or/orconn_event.c index d0a06aa040..7ed6b5fdfa 100644 --- a/src/core/or/orconn_event.c +++ b/src/core/or/orconn_event.c @@ -94,6 +94,6 @@ orconn_status_publish(orconn_status_msg_t *msg) const subsys_fns_t sys_orconn_event = { .name = "orconn_event", .supported = true, - .level = -33, + .level = 22, .add_pubsub = orconn_add_pubsub, }; diff --git a/src/feature/control/btrack.c b/src/feature/control/btrack.c index 874150ee13..3595af0fcc 100644 --- a/src/feature/control/btrack.c +++ b/src/feature/control/btrack.c @@ -57,7 +57,7 @@ btrack_add_pubsub(pubsub_connector_t *connector) const subsys_fns_t sys_btrack = { .name = "btrack", .supported = true, - .level = -30, + .level = 55, .initialize = btrack_init, .shutdown = btrack_fini, .add_pubsub = btrack_add_pubsub, diff --git a/src/lib/compress/compress.c b/src/lib/compress/compress.c index c62d7d5d2a..84e9601920 100644 --- a/src/lib/compress/compress.c +++ b/src/lib/compress/compress.c @@ -695,6 +695,6 @@ subsys_compress_initialize(void) const subsys_fns_t sys_compress = { .name = "compress", .supported = true, - .level = -70, + .level = -55, .initialize = subsys_compress_initialize, }; diff --git a/src/lib/net/network_sys.c b/src/lib/net/network_sys.c index 012fc56bba..f0421385b7 100644 --- a/src/lib/net/network_sys.c +++ b/src/lib/net/network_sys.c @@ -39,7 +39,7 @@ const subsys_fns_t sys_network = { .name = "network", /* Network depends on logging, and a lot of other modules depend on network. */ - .level = -80, + .level = -55, .supported = true, .initialize = subsys_network_initialize, .shutdown = subsys_network_shutdown, From 691d271b2e2e4d3f7c80a86f6de59f016171b8ea Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Feb 2020 12:55:05 -0500 Subject: [PATCH 2/8] Re-order thread initialization to follow logging, and remove a comment. lib/log no longer uses lib/thread; lib/log only uses lib/lock, which is at a lower level. --- src/app/main/subsystem_list.c | 2 +- src/lib/thread/compat_threads.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/main/subsystem_list.c b/src/app/main/subsystem_list.c index 6784ebe8a0..bb15b17365 100644 --- a/src/app/main/subsystem_list.c +++ b/src/app/main/subsystem_list.c @@ -46,8 +46,8 @@ const subsys_fns_t *tor_subsystems[] = { &sys_torerr, &sys_wallclock, - &sys_threads, &sys_logging, + &sys_threads, &sys_time, diff --git a/src/lib/thread/compat_threads.c b/src/lib/thread/compat_threads.c index d56e8a3f76..21125bddad 100644 --- a/src/lib/thread/compat_threads.c +++ b/src/lib/thread/compat_threads.c @@ -130,8 +130,6 @@ subsys_threads_initialize(void) const subsys_fns_t sys_threads = { .name = "threads", .supported = true, - /* Threads is used by logging, which is a diagnostic feature, we want it to - * init right after low-level error handling and approx time. */ - .level = -95, + .level = -89, .initialize = subsys_threads_initialize, }; From 90524de0b268a76665fbe9ddce4878b10c9389c4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Feb 2020 13:14:54 -0500 Subject: [PATCH 3/8] Move winprocess_sys into a new low-level hardening module This code was in our process module, but it doesn't belong there: process is for launching and monitoring subprocesses, not for hardening the current process. This change lets us have our subsystem init order more closely match our dependency order. --- .gitignore | 2 ++ Makefile.am | 2 ++ src/app/main/subsystem_list.c | 2 +- src/include.am | 1 + src/lib/llharden/.may_include | 3 +++ src/lib/llharden/include.am | 19 +++++++++++++++++++ src/lib/llharden/lib_llharden.md | 6 ++++++ .../{process => llharden}/winprocess_sys.c | 2 +- .../{process => llharden}/winprocess_sys.h | 0 src/lib/process/include.am | 6 ++---- 10 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 src/lib/llharden/.may_include create mode 100644 src/lib/llharden/include.am create mode 100644 src/lib/llharden/lib_llharden.md rename src/lib/{process => llharden}/winprocess_sys.c (97%) rename src/lib/{process => llharden}/winprocess_sys.h (100%) diff --git a/.gitignore b/.gitignore index 77610b3193..469bbd39a5 100644 --- a/.gitignore +++ b/.gitignore @@ -186,6 +186,8 @@ uptime-*.json /src/lib/libtor-geoip-testing.a /src/lib/libtor-intmath.a /src/lib/libtor-intmath-testing.a +/src/lib/libtor-llharden.a +/src/lib/libtor-llharden-testing.a /src/lib/libtor-lock.a /src/lib/libtor-lock-testing.a /src/lib/libtor-log.a diff --git a/Makefile.am b/Makefile.am index ac61a990fc..7774995aea 100644 --- a/Makefile.am +++ b/Makefile.am @@ -70,6 +70,7 @@ TOR_UTIL_LIBS = \ src/lib/libtor-wallclock.a \ src/lib/libtor-err.a \ src/lib/libtor-version.a \ + src/lib/libtor-llharden.a \ src/lib/libtor-intmath.a \ src/lib/libtor-ctime.a @@ -104,6 +105,7 @@ TOR_UTIL_TESTING_LIBS = \ src/lib/libtor-wallclock-testing.a \ src/lib/libtor-err-testing.a \ src/lib/libtor-version-testing.a \ + src/lib/libtor-llharden-testing.a \ src/lib/libtor-intmath.a \ src/lib/libtor-ctime-testing.a endif diff --git a/src/app/main/subsystem_list.c b/src/app/main/subsystem_list.c index bb15b17365..84c6e6ec0e 100644 --- a/src/app/main/subsystem_list.c +++ b/src/app/main/subsystem_list.c @@ -24,7 +24,7 @@ #include "lib/log/log_sys.h" #include "lib/net/network_sys.h" #include "lib/process/process_sys.h" -#include "lib/process/winprocess_sys.h" +#include "lib/llharden/winprocess_sys.h" #include "lib/thread/thread_sys.h" #include "lib/time/time_sys.h" #include "lib/tls/tortls_sys.h" diff --git a/src/include.am b/src/include.am index f5f868d23f..657f6e823a 100644 --- a/src/include.am +++ b/src/include.am @@ -19,6 +19,7 @@ include src/lib/fs/include.am include src/lib/geoip/include.am include src/lib/include.libdonna.am include src/lib/intmath/include.am +include src/lib/llharden/include.am include src/lib/lock/include.am include src/lib/log/include.am include src/lib/math/include.am diff --git a/src/lib/llharden/.may_include b/src/lib/llharden/.may_include new file mode 100644 index 0000000000..038237dadf --- /dev/null +++ b/src/lib/llharden/.may_include @@ -0,0 +1,3 @@ +lib/llharden/*.h +lib/subsys/*.h +orconfig.h diff --git a/src/lib/llharden/include.am b/src/lib/llharden/include.am new file mode 100644 index 0000000000..0a4788c7dc --- /dev/null +++ b/src/lib/llharden/include.am @@ -0,0 +1,19 @@ + +noinst_LIBRARIES += src/lib/libtor-llharden.a + +if UNITTESTS_ENABLED +noinst_LIBRARIES += src/lib/libtor-llharden-testing.a +endif + +# ADD_C_FILE: INSERT SOURCES HERE. +src_lib_libtor_llharden_a_SOURCES = \ + src/lib/llharden/winprocess_sys.c + +src_lib_libtor_llharden_testing_a_SOURCES = \ + $(src_lib_libtor_llharden_a_SOURCES) +src_lib_libtor_llharden_testing_a_CPPFLAGS = $(AM_CPPFLAGS) $(TEST_CPPFLAGS) +src_lib_libtor_llharden_testing_a_CFLAGS = $(AM_CFLAGS) $(TEST_CFLAGS) + +# ADD_C_FILE: INSERT HEADERS HERE. +noinst_HEADERS += \ + src/lib/llharden/winprocess_sys.h diff --git a/src/lib/llharden/lib_llharden.md b/src/lib/llharden/lib_llharden.md new file mode 100644 index 0000000000..69e9af5327 --- /dev/null +++ b/src/lib/llharden/lib_llharden.md @@ -0,0 +1,6 @@ +@dir /lib/llharden +@brief lib/llharden: low-level unconditional process hardening + +This module contains process hardening code that we want to run before any +other code, including configuration. It needs to be self-contained, since +nothing else will be initialized at this point. diff --git a/src/lib/process/winprocess_sys.c b/src/lib/llharden/winprocess_sys.c similarity index 97% rename from src/lib/process/winprocess_sys.c rename to src/lib/llharden/winprocess_sys.c index e43a77e467..a5f22c182b 100644 --- a/src/lib/process/winprocess_sys.c +++ b/src/lib/llharden/winprocess_sys.c @@ -8,7 +8,7 @@ #include "orconfig.h" #include "lib/subsys/subsys.h" -#include "lib/process/winprocess_sys.h" +#include "lib/llharden/winprocess_sys.h" #include #include diff --git a/src/lib/process/winprocess_sys.h b/src/lib/llharden/winprocess_sys.h similarity index 100% rename from src/lib/process/winprocess_sys.h rename to src/lib/llharden/winprocess_sys.h diff --git a/src/lib/process/include.am b/src/lib/process/include.am index af5f99617b..18876b3f54 100644 --- a/src/lib/process/include.am +++ b/src/lib/process/include.am @@ -16,8 +16,7 @@ src_lib_libtor_process_a_SOURCES = \ src/lib/process/process_win32.c \ src/lib/process/restrict.c \ src/lib/process/setuid.c \ - src/lib/process/waitpid.c \ - src/lib/process/winprocess_sys.c + src/lib/process/waitpid.c src_lib_libtor_process_testing_a_SOURCES = \ $(src_lib_libtor_process_a_SOURCES) @@ -35,5 +34,4 @@ noinst_HEADERS += \ src/lib/process/process_win32.h \ src/lib/process/restrict.h \ src/lib/process/setuid.h \ - src/lib/process/waitpid.h \ - src/lib/process/winprocess_sys.h + src/lib/process/waitpid.h From 67d921a69617d2b0a9ab56dfe1a6e7631f7ed5eb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Feb 2020 13:37:47 -0500 Subject: [PATCH 4/8] Changes file for ticket 33316 --- changes/ticket33316 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/ticket33316 diff --git a/changes/ticket33316 b/changes/ticket33316 new file mode 100644 index 0000000000..375f4e9c69 --- /dev/null +++ b/changes/ticket33316 @@ -0,0 +1,5 @@ + o Minor bugfixes (initialization): + - Initialize the subsystems in our code in an order more closely + corresponding to their dependencies, so that every system is + initialized before the ones that (theoretically) depend on it. + Fixes bug 33316; bugfix on 0.4.0.1-alpha. From 2e5d555c0e4adb5ea4739ec58124f61c46bd20aa Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 13 Feb 2020 15:11:18 -0500 Subject: [PATCH 5/8] Initialize all subsystems during the unit tests Previously we had initialized only the library subsystems. This made it hard to write some tests, and encouraged people to put their subsystems at a level lower than they really belonged at. Instead, it probably just makes sense to initialize everything before we start the tests. Without this fix, 33316 breaks our tests because of raising the level of the ocirc/orconn events. --- src/test/testing_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/testing_common.c b/src/test/testing_common.c index 655e36e336..16a62bd532 100644 --- a/src/test/testing_common.c +++ b/src/test/testing_common.c @@ -273,7 +273,7 @@ main(int c, const char **v) int loglevel = LOG_ERR; int accel_crypto = 0; - subsystems_init_upto(SUBSYS_LEVEL_LIBS); + subsystems_init(); options = options_new(); From 63b7dabdea6c25cba6604af5d9340799636985b3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Feb 2020 09:34:51 -0500 Subject: [PATCH 6/8] Merge ocirc and orconn events into or subsystem. Pubsub events are not supposed to have their own subsystems; they're supposed to be part of a parent subsystem. --- src/app/main/subsystem_list.c | 4 ---- src/core/or/include.am | 2 -- src/core/or/ocirc_event.c | 11 ++--------- src/core/or/ocirc_event_sys.h | 13 ------------- src/core/or/or_sys.c | 12 ++++++++++++ src/core/or/or_sys.h | 4 ++++ src/core/or/orconn_event.c | 11 ++--------- src/core/or/orconn_event_sys.h | 12 ------------ 8 files changed, 20 insertions(+), 49 deletions(-) delete mode 100644 src/core/or/ocirc_event_sys.h delete mode 100644 src/core/or/orconn_event_sys.h diff --git a/src/app/main/subsystem_list.c b/src/app/main/subsystem_list.c index 84c6e6ec0e..06f39df2e6 100644 --- a/src/app/main/subsystem_list.c +++ b/src/app/main/subsystem_list.c @@ -14,9 +14,7 @@ #include "lib/cc/torint.h" #include "core/mainloop/mainloop_sys.h" -#include "core/or/ocirc_event_sys.h" #include "core/or/or_sys.h" -#include "core/or/orconn_event_sys.h" #include "feature/control/btrack_sys.h" #include "lib/compress/compress_sys.h" #include "lib/crypt_ops/crypto_sys.h" @@ -61,8 +59,6 @@ const subsys_fns_t *tor_subsystems[] = { &sys_mainloop, &sys_or, - &sys_orconn_event, - &sys_ocirc_event, &sys_relay, diff --git a/src/core/or/include.am b/src/core/or/include.am index 4dd251d2e4..3626e76bed 100644 --- a/src/core/or/include.am +++ b/src/core/or/include.am @@ -74,13 +74,11 @@ noinst_HEADERS += \ src/core/or/or_periodic.h \ src/core/or/or_sys.h \ src/core/or/orconn_event.h \ - src/core/or/orconn_event_sys.h \ src/core/or/or_circuit_st.h \ src/core/or/or_connection_st.h \ src/core/or/or_handshake_certs_st.h \ src/core/or/or_handshake_state_st.h \ src/core/or/ocirc_event.h \ - src/core/or/ocirc_event_sys.h \ src/core/or/origin_circuit_st.h \ src/core/or/policies.h \ src/core/or/port_cfg_st.h \ diff --git a/src/core/or/ocirc_event.c b/src/core/or/ocirc_event.c index 6aa7f684e0..fa16459175 100644 --- a/src/core/or/ocirc_event.c +++ b/src/core/or/ocirc_event.c @@ -22,7 +22,7 @@ #include "core/or/cpath_build_state_st.h" #include "core/or/ocirc_event.h" -#include "core/or/ocirc_event_sys.h" +#include "core/or/or_sys.h" #include "core/or/origin_circuit_st.h" #include "lib/subsys/subsys.h" @@ -84,7 +84,7 @@ static dispatch_typefns_t ocirc_cevent_fns = { .fmt_fn = ocirc_cevent_fmt, }; -static int +int ocirc_add_pubsub(struct pubsub_connector_t *connector) { if (DISPATCH_REGISTER_TYPE(connector, ocirc_state, ô_state_fns)) @@ -119,10 +119,3 @@ ocirc_cevent_publish(ocirc_cevent_msg_t *msg) { PUBLISH(ocirc_cevent, msg); } - -const subsys_fns_t sys_ocirc_event = { - .name = "ocirc_event", - .supported = true, - .level = 22, - .add_pubsub = ocirc_add_pubsub, -}; diff --git a/src/core/or/ocirc_event_sys.h b/src/core/or/ocirc_event_sys.h deleted file mode 100644 index abb89b04a0..0000000000 --- a/src/core/or/ocirc_event_sys.h +++ /dev/null @@ -1,13 +0,0 @@ -/* Copyright (c) 2007-2020, The Tor Project, Inc. */ - -/** - * \file ocirc_event_sys.h - * \brief Declare subsystem object for the origin circuit event module. - **/ - -#ifndef TOR_OCIRC_EVENT_SYS_H -#define TOR_OCIRC_EVENT_SYS_H - -extern const struct subsys_fns_t sys_ocirc_event; - -#endif /* !defined(TOR_OCIRC_EVENT_SYS_H) */ diff --git a/src/core/or/or_sys.c b/src/core/or/or_sys.c index 394b7945e1..126f5448cf 100644 --- a/src/core/or/or_sys.c +++ b/src/core/or/or_sys.c @@ -34,10 +34,22 @@ subsys_or_shutdown(void) policies_free_all(); } +static int +subsys_or_add_pubsub(struct pubsub_connector_t *connector) +{ + int rv = 0; + if (orconn_add_pubsub(connector) < 0) + rv = -1; + if (ocirc_add_pubsub(connector) < 0) + rv = -1; + return rv; +} + const struct subsys_fns_t sys_or = { .name = "or", .supported = true, .level = 20, .initialize = subsys_or_initialize, .shutdown = subsys_or_shutdown, + .add_pubsub = subsys_or_add_pubsub, }; diff --git a/src/core/or/or_sys.h b/src/core/or/or_sys.h index 3ae09f7b52..7ee56c8682 100644 --- a/src/core/or/or_sys.h +++ b/src/core/or/or_sys.h @@ -14,4 +14,8 @@ extern const struct subsys_fns_t sys_or; +struct pubsub_connector_t; +int ocirc_add_pubsub(struct pubsub_connector_t *connector); +int orconn_add_pubsub(struct pubsub_connector_t *connector); + #endif /* !defined(TOR_CORE_OR_OR_SYS_H) */ diff --git a/src/core/or/orconn_event.c b/src/core/or/orconn_event.c index 7ed6b5fdfa..c30e2dd22f 100644 --- a/src/core/or/orconn_event.c +++ b/src/core/or/orconn_event.c @@ -22,7 +22,7 @@ #define ORCONN_EVENT_PRIVATE #include "core/or/orconn_event.h" -#include "core/or/orconn_event_sys.h" +#include "core/or/or_sys.h" DECLARE_PUBLISH(orconn_state); DECLARE_PUBLISH(orconn_status); @@ -65,7 +65,7 @@ static dispatch_typefns_t orconn_status_fns = { .fmt_fn = orconn_status_fmt, }; -static int +int orconn_add_pubsub(struct pubsub_connector_t *connector) { if (DISPATCH_REGISTER_TYPE(connector, orconn_state, &orconn_state_fns)) @@ -90,10 +90,3 @@ orconn_status_publish(orconn_status_msg_t *msg) { PUBLISH(orconn_status, msg); } - -const subsys_fns_t sys_orconn_event = { - .name = "orconn_event", - .supported = true, - .level = 22, - .add_pubsub = orconn_add_pubsub, -}; diff --git a/src/core/or/orconn_event_sys.h b/src/core/or/orconn_event_sys.h deleted file mode 100644 index 02f0b8116b..0000000000 --- a/src/core/or/orconn_event_sys.h +++ /dev/null @@ -1,12 +0,0 @@ -/* Copyright (c) 2007-2020, The Tor Project, Inc. */ - -/** - * \file orconn_event_sys.h - * \brief Declare subsystem object for the OR connection event module. - **/ -#ifndef TOR_ORCONN_EVENT_SYS_H -#define TOR_ORCONN_EVENT_SYS_H - -extern const struct subsys_fns_t sys_orconn_event; - -#endif /* !defined(TOR_ORCONN_EVENT_SYS_H) */ From e067cb426a91c58814812bbc883ff7e3e819a3c1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 14 Feb 2020 13:29:15 -0500 Subject: [PATCH 7/8] Move process subsystem after evloop. Process uses evloop, and so should be initialized after it. --- src/app/main/subsystem_list.c | 2 +- src/lib/process/process_sys.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/main/subsystem_list.c b/src/app/main/subsystem_list.c index 06f39df2e6..e32083537f 100644 --- a/src/app/main/subsystem_list.c +++ b/src/app/main/subsystem_list.c @@ -53,9 +53,9 @@ const subsys_fns_t *tor_subsystems[] = { &sys_compress, &sys_network, &sys_tortls, - &sys_process, &sys_evloop, + &sys_process, &sys_mainloop, &sys_or, diff --git a/src/lib/process/process_sys.c b/src/lib/process/process_sys.c index 283064cbfe..015ffadead 100644 --- a/src/lib/process/process_sys.c +++ b/src/lib/process/process_sys.c @@ -26,7 +26,7 @@ subsys_process_shutdown(void) const subsys_fns_t sys_process = { .name = "process", - .level = -35, + .level = -18, .supported = true, .initialize = subsys_process_initialize, .shutdown = subsys_process_shutdown From 0c18c94bb1efd9db42748f942117e19e058d3410 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 21 Feb 2020 16:23:17 -0500 Subject: [PATCH 8/8] extend ticket33316 changes file to include more changes --- changes/ticket33316 | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/changes/ticket33316 b/changes/ticket33316 index 375f4e9c69..25b0444078 100644 --- a/changes/ticket33316 +++ b/changes/ticket33316 @@ -3,3 +3,13 @@ corresponding to their dependencies, so that every system is initialized before the ones that (theoretically) depend on it. Fixes bug 33316; bugfix on 0.4.0.1-alpha. + + o Minor features (tests): + - Initialize all subsystems at the beginning of our unit test harness, + to avoid crashes due to uninitialized subsystems. + Follow-up from ticket 33316. + + o Code simplification and refactoring: + - Merge the orconn and ocirc events into the "core" subsystem, which + manages or connections and origin circuits. Previously they + were isolated in subsystems of their own.