From 4541a590192a97d56c7a303492a2bc1bbeacb441 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 28 Oct 2019 09:05:02 -0400 Subject: [PATCH 01/19] lib/cc: Define the standard C macro pasting trick in one place. This file is a workaround for the issue that if you say `a ## b` to create a token that is the name of a macro, the C preprocessor won't expand that macro. So you can't say this: #define FOO__SQUARE(x) ((x)*(x)) #define FOO__CUBE(x) ((x)*(x)*(x)) #define FOO(func, x) FOO__##func(x) Instead, the standard C trick is to add a layer of indirection: #define PASTE(a,b) PASTE__(a,b) #define PASTE__(a,b) a ## b #define FOO__SQUARE(x) ((x)*(x)) #define FOO__CUBE(x) ((x)*(x)*(x)) #define FOO(func, x) PASTE(FOO__, func)(x) We should use this kind of trick sparingly, since it gets confusing. --- src/lib/cc/include.am | 1 + src/lib/cc/tokpaste.h | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 src/lib/cc/tokpaste.h diff --git a/src/lib/cc/include.am b/src/lib/cc/include.am index 1aa722dd82..d2a415e956 100644 --- a/src/lib/cc/include.am +++ b/src/lib/cc/include.am @@ -3,4 +3,5 @@ noinst_HEADERS += \ src/lib/cc/compat_compiler.h \ src/lib/cc/ctassert.h \ + src/lib/cc/tokpaste.h \ src/lib/cc/torint.h diff --git a/src/lib/cc/tokpaste.h b/src/lib/cc/tokpaste.h new file mode 100644 index 0000000000..e7ddbffc6a --- /dev/null +++ b/src/lib/cc/tokpaste.h @@ -0,0 +1,30 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * @file tokpaste.h + * @brief Token-pasting macros. + **/ + +#ifndef TOR_LIB_CC_TOKPASTE_H +#define TOR_LIB_CC_TOKPASTE_H + +/** + * Concatenate `a` and `b` in a way that allows their result itself to be + * expanded by the preprocessor. + * + * Ordinarily you could just say `a ## b` in a macro definition. But doing so + * results in a symbol which the preprocessor will not then expand. If you + * wanted to use `a ## b` to create the name of a macro and have the + * preprocessor expand _that_ macro, you need to have another level of + * indirection, as this macro provides. + **/ +#define PASTE(a,b) PASTE__(a,b) + +/** Helper for PASTE(). */ +#define PASTE__(a,b) a ## b + +#endif /* !defined(TOR_LIB_CC_TOKPASTE_H) */ From 19cac400d9cf7dcc274c583bb3e942b2682c3663 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 26 Oct 2019 17:46:16 -0400 Subject: [PATCH 02/19] Macros for declaring configuration structs and variable-tables In our old design, we had to declare configuration structures (like or_options_t) and variable tables (like option_vars_) separately, and we used some magic to enforce their consistency (see conftesting.h). With this design, we write a single definition for the configuration object and its fields, and use C macros to expand it either into a structure, or a variable table. Since they are both made from the same source, they can't become inconsistent. The two designs can coexist happily, and we can migrate from one to the other at our convenience. --- src/feature/nodelist/routerset.h | 1 + src/lib/conf/confdecl.h | 166 +++++++++++++++++++++++++++++++ src/lib/conf/include.am | 1 + 3 files changed, 168 insertions(+) create mode 100644 src/lib/conf/confdecl.h diff --git a/src/feature/nodelist/routerset.h b/src/feature/nodelist/routerset.h index dc6ce0b667..6bd97f9422 100644 --- a/src/feature/nodelist/routerset.h +++ b/src/feature/nodelist/routerset.h @@ -46,6 +46,7 @@ int routerset_len(const routerset_t *set); struct var_type_def_t; extern const struct var_type_def_t ROUTERSET_type_defn; +typedef routerset_t config_decl_ROUTERSET; #ifdef ROUTERSET_PRIVATE #include "lib/container/bitarray.h" diff --git a/src/lib/conf/confdecl.h b/src/lib/conf/confdecl.h new file mode 100644 index 0000000000..22812f046e --- /dev/null +++ b/src/lib/conf/confdecl.h @@ -0,0 +1,166 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * @file confdecl.h + * @brief Macros for generating a configuration struct from a list + * of its individual fields. + * + * This header defines three important macros: BEGIN_CONF_STRUCT(), + * END_CONF_STRUCT(), and CONF_VAR(). They're meant to be used together to + * define a configuration structure and the means for encoding and decoding + * it. + * + * To use them, make a new header with a name like `MOD_options.inc`. Start + * it with a BEGIN_CONF_STRUCT(), then define your variables with CONF_VAR(), + * then end the header with END_CONF_STRUCT(), as in: + * + * BEGIN_CONF_STRUCT(module_options_t) + * CONF_VAR(ModuleIsActive, BOOLEAN, 0, "1") + * END_CONF_STRUCT(module_options_t) + * + * Once you've done that, you can use that header to define a configuration + * structure by saying: + * + * typedef struct module_options_t module_options_t; + * #define CONF_CONTEXT STRUCT + * #include "MOD_options.inc" + * #undef CONF_CONTEXT + * + * And you can define your field definition table by saying: + * + * #define CONF_CONTEXT TABLE + * #include "MOD_options.inc" + * #undef CONF_CONTEXT + * + * The two above snippets will define a structure called `module_options_t` + * with appropriate members, and a table of config_var_t objects called + * `module_options_t_vars[]`. + **/ + +#ifndef TOR_LIB_CONF_CONFDECL_H +#define TOR_LIB_CONF_CONFDECL_H + +#undef CONF_CONTEXT +#include "lib/cc/tokpaste.h" + +/** + * Begin the definition of a configuration object called `name`. + **/ +#define BEGIN_CONF_STRUCT(name) \ + PASTE(BEGIN_CONF_STRUCT__, CONF_CONTEXT)(name) +/** + * End the definition of a configuration object called `name`. + **/ +#define END_CONF_STRUCT(name) \ + PASTE(END_CONF_STRUCT__, CONF_CONTEXT)(name) +/** + * Declare a single configuration field with name `varname`, type `vartype`, + * flags `varflags`, and initial value `initval`. + **/ +#define CONF_VAR(varname, vartype, varflags, initval) \ + PASTE(CONF_VAR__, CONF_CONTEXT)(varname, vartype, varflags, initval) + +#ifndef COCCI +/** + * @defgroup STRUCT_MACROS Internal macros: struct definitions. + * Implementation helpers: the regular confdecl macros expand to these + * when CONF_CONTEXT is defined to STRUCT. Don't use them directly. + * @{*/ +#define BEGIN_CONF_STRUCT__STRUCT(name) \ + struct name { \ + uint32_t magic; +#define END_CONF_STRUCT__STRUCT(name) \ + }; +#define CONF_VAR__STRUCT(varname, vartype, varflags, initval) \ + config_decl_ ## vartype varname; +/** @} */ + +/** + * @defgroup TABLE_MACROS Internal macros: table definitions. + * Implementation helpers: the regular confdecl macros expand to these + * when CONF_CONTEXT is defined to TABLE. Don't use them directly. + * @{*/ +#define BEGIN_CONF_STRUCT__TABLE(structname) \ + /* We use this typedef so we can refer to the config type */ \ + /* without having its name as a macro argument to CONF_VAR. */ \ + typedef struct structname config_var_reference__obj; \ + static const config_var_t structname##_vars[] = { +#define END_CONF_STRUCT__TABLE(structname) \ + { .member = { .name = NULL } } \ + }; +#define CONF_VAR__TABLE(varname, vartype, varflags, initval) \ + { \ + .member = \ + { .name = #varname, \ + .type = CONFIG_TYPE_EXTENDED, \ + .type_def = &vartype ## _type_defn, \ + .offset=offsetof(config_var_reference__obj, varname), \ + }, \ + .flags = varflags, \ + .initvalue = initval \ + }, +/**@}*/ +#endif + +/** Type aliases for the "commonly used" configuration types. + * + * Defining them in this way allows our CONF_VAR__STRUCT() macro to declare + * structure members corresponding to the configuration types. For example, + * when the macro sees us declare a configuration option "foo" of type STRING, + * it can emit `config_decl_STRING foo;`, which is an alias for `char *foo`. + */ +/**{*/ +typedef char *config_decl_STRING; +typedef char *config_decl_FILENAME; +/* Yes, "POSINT" is really an int, and not an unsigned int. For + * historical reasons, many configuration values are restricted + * to the range [0,INT_MAX], and stored in signed ints. + */ +typedef int config_decl_POSINT; +typedef uint64_t config_decl_UINT64; +typedef int config_decl_INT; +typedef int config_decl_INTERVAL; +typedef int config_decl_MSEC_INTERVAL; +typedef uint64_t config_decl_MEMUNIT; +typedef double config_decl_DOUBLE; +typedef int config_decl_BOOL; +typedef int config_decl_AUTOBOOL; +typedef time_t config_decl_ISOTIME; +typedef struct smartlist_t config_decl_CSV; +typedef int config_decl_CSV_INTERVAL; +typedef struct config_line_t *config_decl_LINELIST; +typedef struct config_line_t *config_decl_LINELIST_V; +typedef struct nonexistent_struct *config_decl_LINELIST_S; +/**@}*/ + +struct var_type_def_t; + +/* Forward declarations for configuration type definitions. These are used by + * the CONF_VAR__TABLE macro to set the definition of each variable type + * correctly. + */ +/**@{*/ +extern const struct var_type_def_t STRING_type_defn; +extern const struct var_type_def_t FILENAME_type_defn; +extern const struct var_type_def_t POSINT_type_defn; +extern const struct var_type_def_t UINT64_type_defn; +extern const struct var_type_def_t INT_type_defn; +extern const struct var_type_def_t INTERVAL_type_defn; +extern const struct var_type_def_t MSEC_INTERVAL_type_defn; +extern const struct var_type_def_t MEMUNIT_type_defn; +extern const struct var_type_def_t DOUBLE_type_defn; +extern const struct var_type_def_t BOOL_type_defn; +extern const struct var_type_def_t AUTOBOOL_type_defn; +extern const struct var_type_def_t ISOTIME_type_defn; +extern const struct var_type_def_t CSV_type_defn; +extern const struct var_type_def_t CSV_INTERVAL_type_defn; +extern const struct var_type_def_t LINELIST_type_defn; +extern const struct var_type_def_t LINELIST_V_type_defn; +extern const struct var_type_def_t LINELIST_S_type_defn; +/**@}*/ + +#endif /* !defined(TOR_LIB_CONF_CONFDECL_H) */ diff --git a/src/lib/conf/include.am b/src/lib/conf/include.am index cb7126184d..cb0b83fa64 100644 --- a/src/lib/conf/include.am +++ b/src/lib/conf/include.am @@ -1,6 +1,7 @@ # ADD_C_FILE: INSERT HEADERS HERE. noinst_HEADERS += \ + src/lib/conf/confdecl.h \ src/lib/conf/conftesting.h \ src/lib/conf/conftypes.h \ src/lib/conf/confmacros.h From 683d4c2bc8379084062dfd79d679e89c2713ff50 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 15:58:14 -0400 Subject: [PATCH 03/19] Add FOO_type_defn globals so that confdecl.h can refer to them. --- src/lib/conf/confdecl.h | 1 + src/lib/confmgt/type_defs.c | 121 +++++++++++++++++++++++------------- 2 files changed, 80 insertions(+), 42 deletions(-) diff --git a/src/lib/conf/confdecl.h b/src/lib/conf/confdecl.h index 22812f046e..f6f9dc527b 100644 --- a/src/lib/conf/confdecl.h +++ b/src/lib/conf/confdecl.h @@ -161,6 +161,7 @@ extern const struct var_type_def_t CSV_INTERVAL_type_defn; extern const struct var_type_def_t LINELIST_type_defn; extern const struct var_type_def_t LINELIST_V_type_defn; extern const struct var_type_def_t LINELIST_S_type_defn; +extern const struct var_type_def_t OBSOLETE_type_defn; /**@}*/ #endif /* !defined(TOR_LIB_CONF_CONFDECL_H) */ diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index f39f1c3d8d..0bf82ee934 100644 --- a/src/lib/confmgt/type_defs.c +++ b/src/lib/confmgt/type_defs.c @@ -17,6 +17,7 @@ #include "orconfig.h" #include "lib/conf/conftypes.h" +#include "lib/conf/confdecl.h" #include "lib/confmgt/typedvar.h" #include "lib/confmgt/type_defs.h" #include "lib/confmgt/unitparse.h" @@ -720,50 +721,86 @@ static const var_type_fns_t ignore_fns = { .encode = ignore_encode, }; +const var_type_def_t STRING_type_defn = { + .name="String", .fns=&string_fns }; +const var_type_def_t FILENAME_type_defn = { + .name="Filename", .fns=&string_fns }; +const var_type_def_t INT_type_defn = { + .name="SignedInteger", .fns=&int_fns, + .params=&INT_PARSE_UNRESTRICTED }; +const var_type_def_t POSINT_type_defn = { + .name="Integer", .fns=&int_fns, + .params=&INT_PARSE_POSINT }; +const var_type_def_t UINT64_type_defn = { + .name="Integer", .fns=&uint64_fns, }; +const var_type_def_t MEMUNIT_type_defn = { + .name="DataSize", .fns=&memunit_fns, + .params=&memory_units }; +const var_type_def_t INTERVAL_type_defn = { + .name="TimeInterval", .fns=&interval_fns, + .params=&time_units }; +const var_type_def_t MSEC_INTERVAL_type_defn = { + .name="TimeMsecInterval", + .fns=&interval_fns, + .params=&time_msec_units }; +const var_type_def_t DOUBLE_type_defn = { + .name="Float", .fns=&double_fns, }; +const var_type_def_t BOOL_type_defn = { + .name="Boolean", .fns=&enum_fns, + .params=&enum_table_bool }; +const var_type_def_t AUTOBOOL_type_defn = { + .name="Boolean+Auto", .fns=&enum_fns, + .params=&enum_table_autobool }; +const var_type_def_t ISOTIME_type_defn = { + .name="Time", .fns=&time_fns, }; +const var_type_def_t CSV_type_defn = { + .name="CommaList", .fns=&csv_fns, }; +const var_type_def_t CSV_INTERVAL_type_defn = { + .name="TimeInterval", + .fns=&legacy_csv_interval_fns, }; +const var_type_def_t LINELIST_type_defn = { + .name="LineList", .fns=&linelist_fns, + .flags=CFLG_NOREPLACE }; +/* + * A "linelist_s" is a derived view of a linelist_v: inspecting + * it gets part of a linelist_v, and setting it adds to the linelist_v. + */ +const var_type_def_t LINELIST_S_type_defn = { + .name="Dependent", .fns=&linelist_s_fns, + .flags=CFLG_NOREPLACE| + /* The operations we disable here are + * handled by the linelist_v. */ + CFLG_NOCOPY|CFLG_NOCMP|CFLG_NODUMP }; +const var_type_def_t LINELIST_V_type_defn = { + .name="Virtual", .fns=&linelist_v_fns, + .flags=CFLG_NOREPLACE|CFLG_NOSET }; +const var_type_def_t OBSOLETE_type_defn = { + .name="Obsolete", .fns=&ignore_fns, + .flags=CFLG_GROUP_OBSOLETE, +}; + /** * Table mapping conf_type_t values to var_type_def_t objects. **/ -static const var_type_def_t type_definitions_table[] = { - [CONFIG_TYPE_STRING] = { .name="String", .fns=&string_fns }, - [CONFIG_TYPE_FILENAME] = { .name="Filename", .fns=&string_fns }, - [CONFIG_TYPE_INT] = { .name="SignedInteger", .fns=&int_fns, - .params=&INT_PARSE_UNRESTRICTED }, - [CONFIG_TYPE_POSINT] = { .name="Integer", .fns=&int_fns, - .params=&INT_PARSE_POSINT }, - [CONFIG_TYPE_UINT64] = { .name="Integer", .fns=&uint64_fns, }, - [CONFIG_TYPE_MEMUNIT] = { .name="DataSize", .fns=&memunit_fns, - .params=&memory_units }, - [CONFIG_TYPE_INTERVAL] = { .name="TimeInterval", .fns=&interval_fns, - .params=&time_units }, - [CONFIG_TYPE_MSEC_INTERVAL] = { .name="TimeMsecInterval", - .fns=&interval_fns, - .params=&time_msec_units }, - [CONFIG_TYPE_DOUBLE] = { .name="Float", .fns=&double_fns, }, - [CONFIG_TYPE_BOOL] = { .name="Boolean", .fns=&enum_fns, - .params=&enum_table_bool }, - [CONFIG_TYPE_AUTOBOOL] = { .name="Boolean+Auto", .fns=&enum_fns, - .params=&enum_table_autobool }, - [CONFIG_TYPE_ISOTIME] = { .name="Time", .fns=&time_fns, }, - [CONFIG_TYPE_CSV] = { .name="CommaList", .fns=&csv_fns, }, - [CONFIG_TYPE_CSV_INTERVAL] = { .name="TimeInterval", - .fns=&legacy_csv_interval_fns, }, - [CONFIG_TYPE_LINELIST] = { .name="LineList", .fns=&linelist_fns, - .flags=CFLG_NOREPLACE }, - /* - * A "linelist_s" is a derived view of a linelist_v: inspecting - * it gets part of a linelist_v, and setting it adds to the linelist_v. - */ - [CONFIG_TYPE_LINELIST_S] = { .name="Dependent", .fns=&linelist_s_fns, - .flags=CFLG_NOREPLACE| - /* The operations we disable here are - * handled by the linelist_v. */ - CFLG_NOCOPY|CFLG_NOCMP|CFLG_NODUMP }, - [CONFIG_TYPE_LINELIST_V] = { .name="Virtual", .fns=&linelist_v_fns, - .flags=CFLG_NOREPLACE|CFLG_NOSET }, - [CONFIG_TYPE_OBSOLETE] = { - .name="Obsolete", .fns=&ignore_fns, - .flags=CFLG_GROUP_OBSOLETE, - } +static const var_type_def_t *type_definitions_table[] = { + [CONFIG_TYPE_STRING] = &STRING_type_defn, + [CONFIG_TYPE_FILENAME] = &FILENAME_type_defn, + [CONFIG_TYPE_INT] = &INT_type_defn, + [CONFIG_TYPE_POSINT] = &POSINT_type_defn, + [CONFIG_TYPE_UINT64] = &UINT64_type_defn, + [CONFIG_TYPE_MEMUNIT] = &MEMUNIT_type_defn, + [CONFIG_TYPE_INTERVAL] = &INTERVAL_type_defn, + [CONFIG_TYPE_MSEC_INTERVAL] = &MSEC_INTERVAL_type_defn, + [CONFIG_TYPE_DOUBLE] = &DOUBLE_type_defn, + [CONFIG_TYPE_BOOL] = &BOOL_type_defn, + [CONFIG_TYPE_AUTOBOOL] = &AUTOBOOL_type_defn, + [CONFIG_TYPE_ISOTIME] = &ISOTIME_type_defn, + [CONFIG_TYPE_CSV] = &CSV_type_defn, + [CONFIG_TYPE_CSV_INTERVAL] = &CSV_INTERVAL_type_defn, + [CONFIG_TYPE_LINELIST] = &LINELIST_type_defn, + [CONFIG_TYPE_LINELIST_S] = &LINELIST_S_type_defn, + [CONFIG_TYPE_LINELIST_V] = &LINELIST_V_type_defn, + [CONFIG_TYPE_OBSOLETE] = &OBSOLETE_type_defn, }; /** @@ -777,5 +814,5 @@ lookup_type_def(config_type_t type) tor_assert(t >= 0); if (t >= (int)ARRAY_LENGTH(type_definitions_table)) return NULL; - return &type_definitions_table[t]; + return type_definitions_table[t]; } From 1d108894e7abd85d2ba0beeb69c0ee6071521a6c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 10:47:29 -0400 Subject: [PATCH 04/19] Define a low-level version of the confdecl table macros This version uses the enum rather than the type definitions, to avoid layering violations and linking problems. --- src/lib/conf/confdecl.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/lib/conf/confdecl.h b/src/lib/conf/confdecl.h index f6f9dc527b..19b6f85090 100644 --- a/src/lib/conf/confdecl.h +++ b/src/lib/conf/confdecl.h @@ -39,6 +39,11 @@ * The two above snippets will define a structure called `module_options_t` * with appropriate members, and a table of config_var_t objects called * `module_options_t_vars[]`. + * + * For lower-level modules, you can say `#define CONF_TABLE LL_TABLE`, and get + * a table definition suitable for use in modules that are at a lower level + * than lib/confmgt. Note that the types for these tables cannot include any + * extended types. **/ #ifndef TOR_LIB_CONF_CONFDECL_H @@ -104,6 +109,31 @@ .initvalue = initval \ }, /**@}*/ + +/** + * @defgroup LL_TABLE_MACROS Internal macros: low-level table definitions. + * Implementation helpers: the regular confdecl macros expand to these + * when CONF_CONTEXT is defined to LL_TABLE. Don't use them directly. + * @{*/ +#define BEGIN_CONF_STRUCT__LL_TABLE(structname) \ + /* We use this typedef so we can refer to the config type */ \ + /* without having its name as a macro argument to CONF_VAR. */ \ + typedef struct structname config_var_reference__obj; \ + static const config_var_t structname##_vars[] = { +#define END_CONF_STRUCT__LL_TABLE(structname) \ + { .member = { .name = NULL } } \ + }; +#define CONF_VAR__LL_TABLE(varname, vartype, varflags, initval) \ + { \ + .member = \ + { .name = #varname, \ + .type = CONFIG_TYPE_ ## vartype, \ + .offset=offsetof(config_var_reference__obj, varname), \ + }, \ + .flags = varflags, \ + .initvalue = initval \ + }, +/**@}*/ #endif /** Type aliases for the "commonly used" configuration types. From 5914addcbc5da7ce2ae4e705090ccefd71f27007 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 26 Oct 2019 20:21:02 -0400 Subject: [PATCH 05/19] Teach doxygen about option-declaration code. These settings allow doxygen to find and understand doxygen comments in configuration definitions made using the macros in confdecl.h. --- Doxyfile.in | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Doxyfile.in b/Doxyfile.in index 2e1a6d0c23..a087012c71 100644 --- a/Doxyfile.in +++ b/Doxyfile.in @@ -318,7 +318,7 @@ OPTIMIZE_OUTPUT_SLICE = NO # Note that for custom extensions you also need to set FILE_PATTERNS otherwise # the files are not read by doxygen. -EXTENSION_MAPPING = dox=C h=C c=C +EXTENSION_MAPPING = dox=C h=C c=C inc=C # If the MARKDOWN_SUPPORT tag is enabled then doxygen pre-processes all comments # according to the Markdown format, which allows for more readable @@ -845,7 +845,8 @@ INPUT_ENCODING = UTF-8 FILE_PATTERNS = *.c \ *.h \ - *.dox + *.dox \ + *.inc # The RECURSIVE tag can be used to specify whether or not subdirectories should # be searched for input files as well. @@ -2110,7 +2111,10 @@ INCLUDE_FILE_PATTERNS = PREDEFINED = "MOCK_IMPL(a,b,c)=a b c" \ "MOCK_DECL(a,b,c)=a b c" \ - __attribute__(x)= + __attribute__(x)= \ + "BEGIN_CONF_STRUCT(x)=struct x {" \ + "END_CONF_STRUCT(x)=};" \ + "CONF_VAR(a,b,c,d)=b a;" # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this From 86389893080d86f6437476c3ff6f3fb07ad3bd99 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 10:15:22 -0400 Subject: [PATCH 06/19] Expose get_obj() functions from confmgt.c. The subsysmgr code wants to use them. --- src/lib/confmgt/confmgt.c | 4 ++-- src/lib/confmgt/confmgt.h | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/lib/confmgt/confmgt.c b/src/lib/confmgt/confmgt.c index 9377736110..1c1a1595ec 100644 --- a/src/lib/confmgt/confmgt.c +++ b/src/lib/confmgt/confmgt.c @@ -237,7 +237,7 @@ config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) * to configuration objects for other modules. This function gets * the sub-object for a particular module. */ -STATIC void * +void * config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) { tor_assert(mgr); @@ -256,7 +256,7 @@ config_mgr_get_obj_mutable(const config_mgr_t *mgr, void *toplevel, int idx) } /** As config_mgr_get_obj_mutable(), but return a const pointer. */ -STATIC const void * +const void * config_mgr_get_obj(const config_mgr_t *mgr, const void *toplevel, int idx) { return config_mgr_get_obj_mutable(mgr, (void*)toplevel, idx); diff --git a/src/lib/confmgt/confmgt.h b/src/lib/confmgt/confmgt.h index f565742c55..11f0de03a1 100644 --- a/src/lib/confmgt/confmgt.h +++ b/src/lib/confmgt/confmgt.h @@ -123,13 +123,14 @@ bool config_var_is_listable(const config_var_t *var); #define CFG_EQ_LINELIST(a,b,opt) config_lines_eq((a)->opt, (b)->opt) #define CFG_EQ_ROUTERSET(a,b,opt) routerset_equal((a)->opt, (b)->opt) +void *config_mgr_get_obj_mutable(const config_mgr_t *mgr, + void *toplevel, int idx); +const void *config_mgr_get_obj(const config_mgr_t *mgr, + const void *toplevel, int idx); + #ifdef CONFMGT_PRIVATE STATIC void config_reset_line(const config_mgr_t *mgr, void *options, const char *key, int use_defaults); -STATIC void *config_mgr_get_obj_mutable(const config_mgr_t *mgr, - void *toplevel, int idx); -STATIC const void *config_mgr_get_obj(const config_mgr_t *mgr, - const void *toplevel, int idx); #endif /* defined(CONFMGT_PRIVATE) */ #endif /* !defined(TOR_CONFMGT_H) */ From a7cfddc8d18c39be8fb212ee2a96da2d1905d9c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 09:49:55 -0400 Subject: [PATCH 07/19] Make a new structure for tracking subsystem status. We used to have only one boolean per subsystem, but we're about to have a little more information. --- src/app/main/subsysmgr.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/app/main/subsysmgr.c b/src/app/main/subsysmgr.c index 1f4bc840f2..5fc298dbf6 100644 --- a/src/app/main/subsysmgr.c +++ b/src/app/main/subsysmgr.c @@ -31,12 +31,22 @@ **/ static bool subsystem_array_validated = false; +/** + * Runtime status of a single subsystem. + **/ +typedef struct subsys_status_t { + /** True if the given subsystem is initialized. */ + bool initialized; +} subsys_status_t; + +/** An overestimate of the number of subsystems. */ +#define N_SYS_STATUS 128 /** * True if a given subsystem is initialized. Expand this array if there * are more than this number of subsystems. (We'd rather not * dynamically allocate in this module.) **/ -static bool sys_initialized[128]; +static subsys_status_t sys_status[N_SYS_STATUS]; /** * Exit with a raw assertion if the subsystems list is inconsistent; @@ -48,8 +58,8 @@ check_and_setup(void) if (subsystem_array_validated) return; - raw_assert(ARRAY_LENGTH(sys_initialized) >= n_tor_subsystems); - memset(sys_initialized, 0, sizeof(sys_initialized)); + raw_assert(ARRAY_LENGTH(sys_status) >= n_tor_subsystems); + memset(sys_status, 0, sizeof(sys_status)); int last_level = MIN_SUBSYS_LEVEL; @@ -97,7 +107,7 @@ subsystems_init_upto(int target_level) continue; if (sys->level > target_level) break; - if (sys_initialized[i]) + if (sys_status[i].initialized) continue; int r = 0; if (sys->initialize) { @@ -112,7 +122,7 @@ subsystems_init_upto(int target_level) sys->name, i); raw_assert_unreached_msg("A subsystem couldn't be initialized."); } - sys_initialized[i] = true; + sys_status[i].initialized = true; } return 0; @@ -132,7 +142,7 @@ subsystems_add_pubsub_upto(pubsub_builder_t *builder, continue; if (sys->level > target_level) break; - if (! sys_initialized[i]) + if (! sys_status[i].initialized) continue; int r = 0; if (sys->add_pubsub) { @@ -186,13 +196,13 @@ subsystems_shutdown_downto(int target_level) continue; if (sys->level <= target_level) break; - if (! sys_initialized[i]) + if (! sys_status[i].initialized) continue; if (sys->shutdown) { log_debug(LD_GENERAL, "Shutting down %s", sys->name); sys->shutdown(); } - sys_initialized[i] = false; + sys_status[i].initialized = false; } } @@ -208,7 +218,7 @@ subsystems_prefork(void) const subsys_fns_t *sys = tor_subsystems[i]; if (!sys->supported) continue; - if (! sys_initialized[i]) + if (! sys_status[i].initialized) continue; if (sys->prefork) { log_debug(LD_GENERAL, "Pre-fork: %s", sys->name); @@ -229,7 +239,7 @@ subsystems_postfork(void) const subsys_fns_t *sys = tor_subsystems[i]; if (!sys->supported) continue; - if (! sys_initialized[i]) + if (! sys_status[i].initialized) continue; if (sys->postfork) { log_debug(LD_GENERAL, "Post-fork: %s", sys->name); @@ -250,7 +260,7 @@ subsystems_thread_cleanup(void) const subsys_fns_t *sys = tor_subsystems[i]; if (!sys->supported) continue; - if (! sys_initialized[i]) + if (! sys_status[i].initialized) continue; if (sys->thread_cleanup) { log_debug(LD_GENERAL, "Thread cleanup: %s", sys->name); From 7ac4f9d5ec1b31a0d4b76ab62c1afc039c8fe627 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 09:28:34 -0400 Subject: [PATCH 08/19] Give subsystems optional config formats and state formats. The formats, when provided, are now added to the global config_mgr_t objects. --- src/app/config/config.c | 2 ++ src/app/config/statefile.c | 3 ++ src/app/main/subsysmgr.c | 68 +++++++++++++++++++++++++++++++++++++- src/app/main/subsysmgr.h | 4 +++ src/lib/subsys/subsys.h | 13 ++++++++ 5 files changed, 89 insertions(+), 1 deletion(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 5f9a55ed17..7317a5d2f1 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -918,6 +918,8 @@ get_options_mgr(void) { if (PREDICT_UNLIKELY(options_mgr == NULL)) { options_mgr = config_mgr_new(&options_format); + int rv = subsystems_register_options_formats(options_mgr); + tor_assert(rv == 0); config_mgr_freeze(options_mgr); } return options_mgr; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index db4d780a78..f6b0915da4 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -45,6 +45,7 @@ #include "feature/relay/routermode.h" #include "lib/sandbox/sandbox.h" #include "app/config/statefile.h" +#include "app/main/subsysmgr.h" #include "lib/encoding/confline.h" #include "lib/net/resolve.h" #include "lib/version/torversion.h" @@ -180,6 +181,8 @@ get_state_mgr(void) { if (PREDICT_UNLIKELY(state_mgr == NULL)) { state_mgr = config_mgr_new(&state_format); + int rv = subsystems_register_state_formats(state_mgr); + tor_assert(rv == 0); config_mgr_freeze(state_mgr); } return state_mgr; diff --git a/src/app/main/subsysmgr.c b/src/app/main/subsysmgr.c index 5fc298dbf6..e5c76b8e55 100644 --- a/src/app/main/subsysmgr.c +++ b/src/app/main/subsysmgr.c @@ -14,10 +14,12 @@ #include "orconfig.h" #include "app/main/subsysmgr.h" +#include "lib/confmgt/confmgt.h" #include "lib/dispatch/dispatch_naming.h" #include "lib/dispatch/msgtypes.h" #include "lib/err/torerr.h" #include "lib/log/log.h" +#include "lib/log/util_bug.h" #include "lib/malloc/malloc.h" #include "lib/pubsub/pubsub_build.h" #include "lib/pubsub/pubsub_connect.h" @@ -37,6 +39,10 @@ static bool subsystem_array_validated = false; typedef struct subsys_status_t { /** True if the given subsystem is initialized. */ bool initialized; + /** Index for this subsystem's options object, or -1 for none. */ + int options_idx; + /** Index for this subsystem's state object, or -1 for none. */ + int state_idx; } subsys_status_t; /** An overestimate of the number of subsystems. */ @@ -48,6 +54,18 @@ typedef struct subsys_status_t { **/ static subsys_status_t sys_status[N_SYS_STATUS]; +/** Set status to a default (not set-up) state. */ +static void +subsys_status_clear(subsys_status_t *status) +{ + if (!status) + return; + memset(status, 0, sizeof(*status)); + status->initialized = false; + status->state_idx = -1; + status->options_idx = -1; +} + /** * Exit with a raw assertion if the subsystems list is inconsistent; * initialize the subsystem_initialized array. @@ -77,6 +95,8 @@ check_and_setup(void) sys->name, i, sys->level, last_level); raw_assert_unreached_msg("There is a bug in subsystem_list.c"); } + subsys_status_clear(&sys_status[i]); + last_level = sys->level; } @@ -202,7 +222,7 @@ subsystems_shutdown_downto(int target_level) log_debug(LD_GENERAL, "Shutting down %s", sys->name); sys->shutdown(); } - sys_status[i].initialized = false; + subsys_status_clear(&sys_status[i]); } } @@ -268,3 +288,49 @@ subsystems_thread_cleanup(void) } } } + +/** + * Register all subsystem-declared options formats in mgr. + * + * Return 0 on success, -1 on failure. + **/ +int +subsystems_register_options_formats(config_mgr_t *mgr) +{ + tor_assert(mgr); + check_and_setup(); + + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + const subsys_fns_t *sys = tor_subsystems[i]; + if (sys->options_format) { + int options_idx = config_mgr_add_format(mgr, sys->options_format); + sys_status[i].options_idx = options_idx; + log_debug(LD_CONFIG, "Added options format for %s with index %d", + sys->name, options_idx); + } + } + return 0; +} + +/** + * Register all subsystem-declared state formats in mgr. + * + * Return 0 on success, -1 on failure. + **/ +int +subsystems_register_state_formats(config_mgr_t *mgr) +{ + tor_assert(mgr); + check_and_setup(); + + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + const subsys_fns_t *sys = tor_subsystems[i]; + if (sys->state_format) { + int state_idx = config_mgr_add_format(mgr, sys->state_format); + sys_status[i].state_idx = state_idx; + log_debug(LD_CONFIG, "Added state format for %s with index %d", + sys->name, state_idx); + } + } + return 0; +} diff --git a/src/app/main/subsysmgr.h b/src/app/main/subsysmgr.h index f8bc83e0ad..54193f9649 100644 --- a/src/app/main/subsysmgr.h +++ b/src/app/main/subsysmgr.h @@ -31,4 +31,8 @@ void subsystems_prefork(void); void subsystems_postfork(void); void subsystems_thread_cleanup(void); +struct config_mgr_t; +int subsystems_register_options_formats(struct config_mgr_t *mgr); +int subsystems_register_state_formats(struct config_mgr_t *mgr); + #endif /* !defined(TOR_SUBSYSMGR_T) */ diff --git a/src/lib/subsys/subsys.h b/src/lib/subsys/subsys.h index 91abdb7d74..1cb3fe94a5 100644 --- a/src/lib/subsys/subsys.h +++ b/src/lib/subsys/subsys.h @@ -14,6 +14,7 @@ #include struct pubsub_connector_t; +struct config_format_t; /** * A subsystem is a part of Tor that is initialized, shut down, configured, @@ -88,6 +89,18 @@ typedef struct subsys_fns_t { **/ void (*shutdown)(void); + /** + * A config_format_t describing all of the torrc fields owned by this + * subsystem. + **/ + const struct config_format_t *options_format; + + /** + * A config_format_t describing all of the DataDir/state fields owned by + * this subsystem. + **/ + const struct config_format_t *state_format; + } subsys_fns_t; /** From 52c0ab4af3ec152c4b78669acf8877ca27d66097 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Oct 2019 10:29:43 -0400 Subject: [PATCH 09/19] Add subsys functions for receiving/flushing states and options. These functions are in the subsystem, not in the config_format_t, since they are about how the format is _used_, not about _what it is_. --- src/app/config/config.c | 3 +- src/app/config/statefile.c | 4 ++ src/app/main/subsysmgr.c | 76 ++++++++++++++++++++++++++++++++++++++ src/app/main/subsysmgr.h | 8 ++++ src/lib/subsys/subsys.h | 29 +++++++++++++++ 5 files changed, 119 insertions(+), 1 deletion(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 7317a5d2f1..8929349709 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -987,7 +987,8 @@ set_options(or_options_t *new_val, char **msg) global_options = old_options; return -1; } - if (options_act(old_options) < 0) { /* acting on the options failed. die. */ + if (subsystems_set_options(get_options_mgr(), new_val) < 0 || + options_act(old_options) < 0) { /* acting on the options failed. die. */ if (! tor_event_loop_shutdown_is_pending()) { log_err(LD_BUG, "Acting on config options left us in a broken state. Dying."); diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index f6b0915da4..5fdb15ace1 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -316,6 +316,9 @@ or_state_set(or_state_t *new_state) tor_assert(new_state); config_free(get_state_mgr(), global_state); global_state = new_state; + if (subsystems_set_state(get_state_mgr(), global_state) < 0) { + ret = -1; + } if (entry_guards_parse_state(global_state, 1, &err)<0) { log_warn(LD_GENERAL,"%s",err); tor_free(err); @@ -519,6 +522,7 @@ or_state_save(time_t now) /* Call everything else that might dirty the state even more, in order * to avoid redundant writes. */ + (void) subsystems_flush_state(get_state_mgr(), global_state); entry_guards_update_state(global_state); rep_hist_update_state(global_state); circuit_build_times_update_state(get_circuit_build_times(), global_state); diff --git a/src/app/main/subsysmgr.c b/src/app/main/subsysmgr.c index e5c76b8e55..bd9af11444 100644 --- a/src/app/main/subsysmgr.c +++ b/src/app/main/subsysmgr.c @@ -334,3 +334,79 @@ subsystems_register_state_formats(config_mgr_t *mgr) } return 0; } + +/** + * Call all appropriate set_options() methods to tell the various subsystems + * about a new set of torrc options. Return 0 on success, -1 on + * nonrecoverable failure. + **/ +int +subsystems_set_options(const config_mgr_t *mgr, struct or_options_t *options) +{ + /* XXXX This does not yet handle reversible option assignment; I'll + * do that later in this branch. */ + + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + const subsys_fns_t *sys = tor_subsystems[i]; + if (sys_status[i].options_idx >= 0 && sys->set_options) { + void *obj = config_mgr_get_obj_mutable(mgr, options, + sys_status[i].options_idx); + int rv = sys->set_options(obj); + if (rv < 0) { + log_err(LD_CONFIG, "Error when handling option for %s; " + "cannot proceed.", sys->name); + return -1; + } + } + } + return 0; +} + +/** + * Call all appropriate set_state() methods to tell the various subsystems + * about an initial DataDir/state file. Return 0 on success, -1 on + * nonrecoverable failure. + **/ +int +subsystems_set_state(const config_mgr_t *mgr, struct or_state_t *state) +{ + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + const subsys_fns_t *sys = tor_subsystems[i]; + if (sys_status[i].state_idx >= 0 && sys->set_state) { + void *obj = config_mgr_get_obj_mutable(mgr, state, + sys_status[i].state_idx); + int rv = sys->set_state(obj); + if (rv < 0) { + log_err(LD_CONFIG, "Error when handling state for %s; " + "cannot proceed.", sys->name); + return -1; + } + } + } + return 0; +} + +/** + * Call all appropriate flush_state() methods to tell the various subsystems + * to update the state objects in state. Return 0 on success, + * -1 on failure. + **/ +int +subsystems_flush_state(const config_mgr_t *mgr, struct or_state_t *state) +{ + int result = 0; + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + const subsys_fns_t *sys = tor_subsystems[i]; + if (sys_status[i].state_idx >= 0 && sys->flush_state) { + void *obj = config_mgr_get_obj_mutable(mgr, state, + sys_status[i].state_idx); + int rv = sys->flush_state(obj); + if (rv < 0) { + log_warn(LD_CONFIG, "Error when flushing state to state object for %s", + sys->name); + result = -1; + } + } + } + return result; +} diff --git a/src/app/main/subsysmgr.h b/src/app/main/subsysmgr.h index 54193f9649..7e5fe76362 100644 --- a/src/app/main/subsysmgr.h +++ b/src/app/main/subsysmgr.h @@ -34,5 +34,13 @@ void subsystems_thread_cleanup(void); struct config_mgr_t; int subsystems_register_options_formats(struct config_mgr_t *mgr); int subsystems_register_state_formats(struct config_mgr_t *mgr); +struct or_options_t; +struct or_state_t; +int subsystems_set_options(const struct config_mgr_t *mgr, + struct or_options_t *options); +int subsystems_set_state(const struct config_mgr_t *mgr, + struct or_state_t *state); +int subsystems_flush_state(const struct config_mgr_t *mgr, + struct or_state_t *state); #endif /* !defined(TOR_SUBSYSMGR_T) */ diff --git a/src/lib/subsys/subsys.h b/src/lib/subsys/subsys.h index 1cb3fe94a5..29a90c049d 100644 --- a/src/lib/subsys/subsys.h +++ b/src/lib/subsys/subsys.h @@ -101,6 +101,35 @@ typedef struct subsys_fns_t { **/ const struct config_format_t *state_format; + /** + * Receive an options object as defined by options_format. Return 0 + * on success, -1 on failure. + * + * It is safe to store the pointer to the object until set_options() + * is called again. */ + int (*set_options)(void *); + + /* XXXX Add an implementation for options_act_reversible() later in this + * branch. */ + + /** + * Receive a state object as defined by state_format. Return 0 on success, + * -1 on failure. + * + * It is safe to store the pointer to the object; set_state() is only + * called on startup. + **/ + int (*set_state)(void *); + + /** + * Update any information that needs to be stored in the provided state + * object (as defined by state_format). Return 0 on success, -1 on failure. + * + * The object provided here will be the same one as provided earlier to + * set_state(). This method is called when we are about to save the state + * to disk. + **/ + int (*flush_state)(void *); } subsys_fns_t; /** From b06e9d8ad58bfe11d2c1f6a921ba059658a578f2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 1 Nov 2019 10:41:02 -0400 Subject: [PATCH 10/19] Add testing-only functions to get the subsystem config/state indices --- src/app/main/subsysmgr.c | 48 ++++++++++++++++++++++++++++++++++++++++ src/app/main/subsysmgr.h | 5 +++++ 2 files changed, 53 insertions(+) diff --git a/src/app/main/subsysmgr.c b/src/app/main/subsysmgr.c index bd9af11444..4189519cad 100644 --- a/src/app/main/subsysmgr.c +++ b/src/app/main/subsysmgr.c @@ -335,6 +335,54 @@ subsystems_register_state_formats(config_mgr_t *mgr) return 0; } +#ifdef TOR_UNIT_TESTS +/** + * Helper: look up the index for sys. Return -1 if the subsystem + * is not recognized. + **/ +static int +subsys_get_idx(const subsys_fns_t *sys) +{ + for (unsigned i = 0; i < n_tor_subsystems; ++i) { + if (sys == tor_subsystems[i]) + return (int)i; + } + return -1; +} + +/** + * Return the current state-manager's index for any state held by the + * subsystem sys. If sys has no options, return -1. + * + * Using raw indices can be error-prone: only do this from the unit + * tests. If you need a way to access another subsystem's configuration, + * that subsystem should provide access functions. + **/ +int +subsystems_get_options_idx(const subsys_fns_t *sys) +{ + int i = subsys_get_idx(sys); + tor_assert(i >= 0); + return sys_status[i].options_idx; +} + +/** + * Return the current state-manager's index for any state held by the + * subsystem sys. If sys has no state, return -1. + * + * Using raw indices can be error-prone: only do this from the unit + * tests. If you need a way to access another subsystem's state + * that subsystem should provide access functions. + **/ +int +subsystems_get_state_idx(const subsys_fns_t *sys) +{ + int i = subsys_get_idx(sys); + tor_assert(i >= 0); + return sys_status[i].state_idx; +} +#endif + /** * Call all appropriate set_options() methods to tell the various subsystems * about a new set of torrc options. Return 0 on success, -1 on diff --git a/src/app/main/subsysmgr.h b/src/app/main/subsysmgr.h index 7e5fe76362..c1138e1ff3 100644 --- a/src/app/main/subsysmgr.h +++ b/src/app/main/subsysmgr.h @@ -43,4 +43,9 @@ int subsystems_set_state(const struct config_mgr_t *mgr, int subsystems_flush_state(const struct config_mgr_t *mgr, struct or_state_t *state); +#ifdef TOR_UNIT_TESTS +int subsystems_get_options_idx(const subsys_fns_t *sys); +int subsystems_get_state_idx(const subsys_fns_t *sys); +#endif + #endif /* !defined(TOR_SUBSYSMGR_T) */ From 8cd3e66d93a7b3f61afc3bc0c8868fb50c85af22 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 26 Oct 2019 18:05:58 -0400 Subject: [PATCH 11/19] Use new configuration architecture for crypto options This is a comparatively simple change. --- src/app/config/config.c | 8 --- src/app/config/or_options_st.h | 4 -- src/app/main/main.c | 10 ---- src/feature/relay/router.c | 9 --- src/lib/crypt_ops/.may_include | 3 + src/lib/crypt_ops/crypto_init.c | 84 +++++++++++++++++++++++++++ src/lib/crypt_ops/crypto_options.inc | 19 ++++++ src/lib/crypt_ops/crypto_options_st.h | 23 ++++++++ src/lib/crypt_ops/include.am | 2 + src/test/test_options.c | 17 +++++- 10 files changed, 145 insertions(+), 34 deletions(-) create mode 100644 src/lib/crypt_ops/crypto_options.inc create mode 100644 src/lib/crypt_ops/crypto_options_st.h diff --git a/src/app/config/config.c b/src/app/config/config.c index 8929349709..480d225e92 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -498,11 +498,8 @@ static const config_var_t option_vars_[] = { #endif /* defined(_WIN32) */ OBSOLETE("Group"), V(GuardLifetime, INTERVAL, "0 minutes"), - V_IMMUTABLE(HardwareAccel, BOOL, "0"), V(HeartbeatPeriod, INTERVAL, "6 hours"), V(MainloopStats, BOOL, "0"), - V_IMMUTABLE(AccelName, STRING, NULL), - V_IMMUTABLE(AccelDir, FILENAME, NULL), V(HashedControlPassword, LINELIST, NULL), OBSOLETE("HidServDirectoryV2"), VAR("HiddenServiceDir", LINELIST_S, RendConfigLines, NULL), @@ -3938,11 +3935,6 @@ options_validate_cb(const void *old_options_, void *options_, char **msg) "testing Tor network!"); } - if (options->AccelName && !options->HardwareAccel) - options->HardwareAccel = 1; - if (options->AccelDir && !options->AccelName) - REJECT("Can't use hardware crypto accelerator dir without engine name."); - if (options_validate_scheduler(options, msg) < 0) { return -1; } diff --git a/src/app/config/or_options_st.h b/src/app/config/or_options_st.h index 5511763daa..a3d63d9208 100644 --- a/src/app/config/or_options_st.h +++ b/src/app/config/or_options_st.h @@ -536,12 +536,8 @@ struct or_options_t { * protocol, is it a warn or an info in our logs? */ int TestSocks; /**< Boolean: when we get a socks connection, do we loudly * log whether it was DNS-leaking or not? */ - int HardwareAccel; /**< Boolean: Should we enable OpenSSL hardware - * acceleration where available? */ /** Token Bucket Refill resolution in milliseconds. */ int TokenBucketRefillInterval; - char *AccelName; /**< Optional hardware acceleration engine name. */ - char *AccelDir; /**< Optional hardware acceleration engine search dir. */ /** Boolean: Do we try to enter from a smallish number * of fixed nodes? */ diff --git a/src/app/main/main.c b/src/app/main/main.c index fad2e0b62f..6029ed3d2d 100644 --- a/src/app/main/main.c +++ b/src/app/main/main.c @@ -592,9 +592,6 @@ tor_init(int argc, char *argv[]) return 1; } - /* The options are now initialised */ - const or_options_t *options = get_options(); - /* Initialize channelpadding and circpad parameters to defaults * until we get a consensus */ channelpadding_new_consensus_params(NULL); @@ -616,13 +613,6 @@ tor_init(int argc, char *argv[]) "and you probably shouldn't."); #endif - if (crypto_global_init(options->HardwareAccel, - options->AccelName, - options->AccelDir)) { - log_err(LD_BUG, "Unable to initialize OpenSSL. Exiting."); - return -1; - } - /* Scan/clean unparseable descriptors; after reading config */ routerparse_init(); diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 7f80b288de..410ed8c2f3 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -887,15 +887,6 @@ init_keys_common(void) if (!key_lock) key_lock = tor_mutex_new(); - /* There are a couple of paths that put us here before we've asked - * openssl to initialize itself. */ - if (crypto_global_init(get_options()->HardwareAccel, - get_options()->AccelName, - get_options()->AccelDir)) { - log_err(LD_BUG, "Unable to initialize OpenSSL. Exiting."); - return -1; - } - return 0; } diff --git a/src/lib/crypt_ops/.may_include b/src/lib/crypt_ops/.may_include index 0739699686..810e777271 100644 --- a/src/lib/crypt_ops/.may_include +++ b/src/lib/crypt_ops/.may_include @@ -1,6 +1,7 @@ orconfig.h lib/arch/*.h lib/cc/*.h +lib/conf/*.h lib/container/*.h lib/crypt_ops/*.h lib/ctime/*.h @@ -17,6 +18,8 @@ lib/testsupport/*.h lib/thread/*.h lib/log/*.h +lib/crypt_ops/*.inc + trunnel/pwbox.h keccak-tiny/*.h diff --git a/src/lib/crypt_ops/crypto_init.c b/src/lib/crypt_ops/crypto_init.c index a16bf4e11a..999eac75f5 100644 --- a/src/lib/crypt_ops/crypto_init.c +++ b/src/lib/crypt_ops/crypto_init.c @@ -23,6 +23,9 @@ #include "lib/crypt_ops/crypto_nss_mgt.h" #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_sys.h" +#include "lib/crypt_ops/crypto_options_st.h" +#include "lib/conf/conftypes.h" +#include "lib/log/util_bug.h" #include "lib/subsys/subsys.h" @@ -252,6 +255,84 @@ subsys_crypto_thread_cleanup(void) crypto_thread_cleanup(); } +/** Magic number for crypto_options_t. */ +#define CRYPTO_OPTIONS_MAGIC 0x68757368 + +/** Invoked before validating crypto options: makes sure that if + * AccelName is set, HardwareAccel is turned on. + **/ +static int +crypto_options_prenormalize(void *arg, char **msg_out) +{ + crypto_options_t *opt = arg; + tor_assert(opt->magic == CRYPTO_OPTIONS_MAGIC); + (void)msg_out; + + // TODO: It would be cleaner to remove this code, but right now the + // tests depend on it. + if (opt->AccelName && !opt->HardwareAccel) + opt->HardwareAccel = 1; + + return 0; +} + +/** + * Return 0 if arg is a valid crypto_options_t. Otherwise return -1 + * and set *msg_out to a freshly allocated error string. + **/ +static int +crypto_options_validate(const void *arg, char **msg_out) +{ + const crypto_options_t *opt = arg; + tor_assert(opt->magic == CRYPTO_OPTIONS_MAGIC); + tor_assert(msg_out); + + if (opt->AccelDir && !opt->AccelName) { + *msg_out = tor_strdup("Can't use hardware crypto accelerator dir " + "without engine name."); + return -1; + } + + return 0; +} + +/* Declare the options field table for crypto_options */ +#define CONF_CONTEXT LL_TABLE +#include "lib/crypt_ops/crypto_options.inc" +#undef CONF_CONTEXT + +/** + * Declares the configuration options for this module. + **/ +static const config_format_t crypto_options_fmt = { + .size = sizeof(crypto_options_t), + .magic = { "crypto_options_t", + CRYPTO_OPTIONS_MAGIC, + offsetof(crypto_options_t, magic) }, + .vars = crypto_options_t_vars, + .pre_normalize_fn = crypto_options_prenormalize, + .validate_fn = crypto_options_validate, + .config_suite_offset = -1, +}; + +/** + * Invoked from subsysmgr.c when a new set of options arrives. + **/ +static int +crypto_set_options(void *arg) +{ + const crypto_options_t *options = arg; + // This call already checks for crypto_global_initialized_, so it + // will only initialize the subsystem the first time it's called. + if (crypto_global_init(options->HardwareAccel, + options->AccelName, + options->AccelDir)) { + log_err(LD_BUG, "Unable to initialize the crypto subsystem. Exiting."); + return -1; + } + return 0; +} + const struct subsys_fns_t sys_crypto = { .name = "crypto", .supported = true, @@ -261,4 +342,7 @@ const struct subsys_fns_t sys_crypto = { .prefork = subsys_crypto_prefork, .postfork = subsys_crypto_postfork, .thread_cleanup = subsys_crypto_thread_cleanup, + + .options_format = &crypto_options_fmt, + .set_options = crypto_set_options, }; diff --git a/src/lib/crypt_ops/crypto_options.inc b/src/lib/crypt_ops/crypto_options.inc new file mode 100644 index 0000000000..5bee0daacd --- /dev/null +++ b/src/lib/crypt_ops/crypto_options.inc @@ -0,0 +1,19 @@ + +/** + * @file crypto_options.inc + * @brief Declare configuration options for the crypto_ops module. + **/ + +/** Holds configuration about our cryptography options. */ +BEGIN_CONF_STRUCT(crypto_options_t) + +/** Should we enable extra OpenSSL hardware acceleration (where available)? */ +CONF_VAR(HardwareAccel, BOOL, CFLG_IMMUTABLE, "0") + +/** Optional OpenSSL hardware-acceleration engine name */ +CONF_VAR(AccelName, STRING, CFLG_IMMUTABLE, NULL) + +/** Optional OpenSSL hardware-acceleration engine search directory. */ +CONF_VAR(AccelDir, FILENAME, CFLG_IMMUTABLE, NULL) + +END_CONF_STRUCT(crypto_options_t) diff --git a/src/lib/crypt_ops/crypto_options_st.h b/src/lib/crypt_ops/crypto_options_st.h new file mode 100644 index 0000000000..8127b41eec --- /dev/null +++ b/src/lib/crypt_ops/crypto_options_st.h @@ -0,0 +1,23 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * @file crypto_options_st.h + * @brief Header for lib/crypt_ops/crypto_options_st.c + **/ + +#ifndef TOR_LIB_CRYPT_OPS_CRYPTO_OPTIONS_ST_H +#define TOR_LIB_CRYPT_OPS_CRYPTO_OPTIONS_ST_H + +#include "lib/conf/confdecl.h" + +#define CONF_CONTEXT STRUCT +#include "lib/crypt_ops/crypto_options.inc" +#undef CONF_CONTEXT + +typedef struct crypto_options_t crypto_options_t; + +#endif /* !defined(TOR_LIB_CRYPT_OPS_CRYPTO_OPTIONS_ST_H) */ diff --git a/src/lib/crypt_ops/include.am b/src/lib/crypt_ops/include.am index 1f58a33d38..7644cab412 100644 --- a/src/lib/crypt_ops/include.am +++ b/src/lib/crypt_ops/include.am @@ -68,6 +68,8 @@ noinst_HEADERS += \ src/lib/crypt_ops/crypto_nss_mgt.h \ src/lib/crypt_ops/crypto_openssl_mgt.h \ src/lib/crypt_ops/crypto_ope.h \ + src/lib/crypt_ops/crypto_options.inc \ + src/lib/crypt_ops/crypto_options_st.h \ src/lib/crypt_ops/crypto_pwbox.h \ src/lib/crypt_ops/crypto_rand.h \ src/lib/crypt_ops/crypto_rsa.h \ diff --git a/src/test/test_options.c b/src/test/test_options.c index c06fb998fb..6a933bec4f 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -17,8 +17,11 @@ #define ROUTERSET_PRIVATE #include "feature/nodelist/routerset.h" #include "core/mainloop/mainloop.h" +#include "app/main/subsysmgr.h" #include "test/log_test_helpers.h" #include "test/resolve_test_helpers.h" +#include "lib/crypt_ops/crypto_options_st.h" +#include "lib/crypt_ops/crypto_sys.h" #include "lib/sandbox/sandbox.h" #include "lib/memarea/memarea.h" @@ -3985,6 +3988,14 @@ test_options_validate__testing_options(void *ignored) tor_free(msg); } +static crypto_options_t * +get_crypto_options(or_options_t *opt) +{ + int idx = subsystems_get_options_idx(&sys_crypto); + tor_assert(idx >= 0); + return config_mgr_get_obj_mutable(get_options_mgr(), opt, idx); +} + static void test_options_validate__accel(void *ignored) { @@ -3997,15 +4008,15 @@ test_options_validate__accel(void *ignored) tdata = get_options_test_data("AccelName foo\n"); ret = options_validate(NULL, tdata->opt, &msg); tt_int_op(ret, OP_EQ, 0); - tt_int_op(tdata->opt->HardwareAccel, OP_EQ, 1); + tt_int_op(get_crypto_options(tdata->opt)->HardwareAccel, OP_EQ, 1); tor_free(msg); free_options_test_data(tdata); tdata = get_options_test_data("AccelName foo\n"); - tdata->opt->HardwareAccel = 2; + get_crypto_options(tdata->opt)->HardwareAccel = 2; ret = options_validate(NULL, tdata->opt, &msg); tt_int_op(ret, OP_EQ, 0); - tt_int_op(tdata->opt->HardwareAccel, OP_EQ, 2); + tt_int_op(get_crypto_options(tdata->opt)->HardwareAccel, OP_EQ, 2); tor_free(msg); free_options_test_data(tdata); From 0d8504e70b0c6ff92f38fc7f4b4c38037d121e69 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 5 Nov 2019 10:04:39 -0500 Subject: [PATCH 12/19] crypto config: do not change the user's value of HardwareAccel. We still interpret "AccelName" as turning on the "HardwareAccel" feature, but we no longer modify the user's options here. Fixes bug 32382; bugfix on 0.2.2.1-alpha when we added openssl engine support. --- changes/ticket32382 | 4 ++++ src/lib/crypt_ops/crypto_init.c | 23 +++-------------------- src/test/test_options.c | 2 +- 3 files changed, 8 insertions(+), 21 deletions(-) create mode 100644 changes/ticket32382 diff --git a/changes/ticket32382 b/changes/ticket32382 new file mode 100644 index 0000000000..85cf07b2ef --- /dev/null +++ b/changes/ticket32382 @@ -0,0 +1,4 @@ + o Minor bugfixes (configuration): + - Avoid changing the user's value of HardwareAccel as stored by SAVECONF, + when AccelName is set but HardwareAccel is not. + Fixes bug 32382; bugfix on 0.2.2.1-alpha. diff --git a/src/lib/crypt_ops/crypto_init.c b/src/lib/crypt_ops/crypto_init.c index 999eac75f5..4b08456197 100644 --- a/src/lib/crypt_ops/crypto_init.c +++ b/src/lib/crypt_ops/crypto_init.c @@ -258,24 +258,6 @@ subsys_crypto_thread_cleanup(void) /** Magic number for crypto_options_t. */ #define CRYPTO_OPTIONS_MAGIC 0x68757368 -/** Invoked before validating crypto options: makes sure that if - * AccelName is set, HardwareAccel is turned on. - **/ -static int -crypto_options_prenormalize(void *arg, char **msg_out) -{ - crypto_options_t *opt = arg; - tor_assert(opt->magic == CRYPTO_OPTIONS_MAGIC); - (void)msg_out; - - // TODO: It would be cleaner to remove this code, but right now the - // tests depend on it. - if (opt->AccelName && !opt->HardwareAccel) - opt->HardwareAccel = 1; - - return 0; -} - /** * Return 0 if arg is a valid crypto_options_t. Otherwise return -1 * and set *msg_out to a freshly allocated error string. @@ -310,7 +292,6 @@ static const config_format_t crypto_options_fmt = { CRYPTO_OPTIONS_MAGIC, offsetof(crypto_options_t, magic) }, .vars = crypto_options_t_vars, - .pre_normalize_fn = crypto_options_prenormalize, .validate_fn = crypto_options_validate, .config_suite_offset = -1, }; @@ -322,9 +303,11 @@ static int crypto_set_options(void *arg) { const crypto_options_t *options = arg; + const bool hardware_accel = options->HardwareAccel || options->AccelName; + // This call already checks for crypto_global_initialized_, so it // will only initialize the subsystem the first time it's called. - if (crypto_global_init(options->HardwareAccel, + if (crypto_global_init(hardware_accel, options->AccelName, options->AccelDir)) { log_err(LD_BUG, "Unable to initialize the crypto subsystem. Exiting."); diff --git a/src/test/test_options.c b/src/test/test_options.c index 6a933bec4f..8aa4bf0906 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -4008,7 +4008,7 @@ test_options_validate__accel(void *ignored) tdata = get_options_test_data("AccelName foo\n"); ret = options_validate(NULL, tdata->opt, &msg); tt_int_op(ret, OP_EQ, 0); - tt_int_op(get_crypto_options(tdata->opt)->HardwareAccel, OP_EQ, 1); + tt_int_op(get_crypto_options(tdata->opt)->HardwareAccel, OP_EQ, 0); tor_free(msg); free_options_test_data(tdata); From 0f0a9bdf332002bb0542dae6bb00e922af5dcf63 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 5 Nov 2019 10:18:47 -0500 Subject: [PATCH 13/19] Stop using "config_suite_offset=-1" to indicate "no config suite." Instead, create a separate "has_config_suite" boolean, so that only top-level formats with config_suites need to declare an offset at all. --- src/app/config/config.c | 1 + src/app/config/statefile.c | 1 + src/feature/dirauth/shared_random_state.c | 1 - src/lib/conf/conftypes.h | 10 ++++++++-- src/lib/confmgt/confmgt.c | 9 +++++++-- src/lib/crypt_ops/crypto_init.c | 1 - src/test/test_confmgr.c | 3 +-- src/test/test_confparse.c | 2 -- 8 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index 480d225e92..c121775c04 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -880,6 +880,7 @@ static const config_format_t options_format = { .legacy_validate_fn = options_validate_cb, .check_transition_fn = options_check_transition_cb, .clear_fn = options_clear_cb, + .has_config_suite = true, .config_suite_offset = offsetof(or_options_t, subconfigs_), }; diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 5fdb15ace1..3e793ff402 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -169,6 +169,7 @@ static const config_format_t state_format = { .vars = state_vars_, .legacy_validate_fn = or_state_validate_cb, .extra = &state_extra_var, + .has_config_suite = true, .config_suite_offset = offsetof(or_state_t, substates_), }; diff --git a/src/feature/dirauth/shared_random_state.c b/src/feature/dirauth/shared_random_state.c index 759b3b8104..bf4302f168 100644 --- a/src/feature/dirauth/shared_random_state.c +++ b/src/feature/dirauth/shared_random_state.c @@ -92,7 +92,6 @@ static const config_format_t state_format = { }, .vars = state_vars, .extra = &state_extra_var, - .config_suite_offset = -1, }; /** Global configuration manager for the shared-random state file */ diff --git a/src/lib/conf/conftypes.h b/src/lib/conf/conftypes.h index d4e2ea218a..dfe51cfba1 100644 --- a/src/lib/conf/conftypes.h +++ b/src/lib/conf/conftypes.h @@ -335,8 +335,14 @@ typedef struct config_format_t { /** If present, extra denotes a LINELIST variable for unrecognized * lines. Otherwise, unrecognized lines are an error. */ const struct_member_t *extra; - /** The position of a config_suite_t pointer within the toplevel object, - * or -1 if there is no such pointer. */ + /** + * If true, this format describes a top-level configuration, with + * a suite containing multiple sub-configuration objects. + */ + bool has_config_suite; + /** The position of a config_suite_t pointer within the toplevel object. + * Ignored unless have_config_suite is true. + */ ptrdiff_t config_suite_offset; } config_format_t; diff --git a/src/lib/confmgt/confmgt.c b/src/lib/confmgt/confmgt.c index 1c1a1595ec..a96c7f96bf 100644 --- a/src/lib/confmgt/confmgt.c +++ b/src/lib/confmgt/confmgt.c @@ -169,9 +169,14 @@ config_mgr_register_fmt(config_mgr_t *mgr, "it had been frozen."); if (object_idx != IDX_TOPLEVEL) { - tor_assertf(fmt->config_suite_offset < 0, + tor_assertf(! fmt->has_config_suite, "Tried to register a toplevel format in a non-toplevel position"); } + if (fmt->config_suite_offset) { + tor_assertf(fmt->has_config_suite, + "config_suite_offset was set, but has_config_suite was not."); + } + tor_assertf(fmt != mgr->toplevel && ! smartlist_contains(mgr->subconfigs, fmt), "Tried to register an already-registered format."); @@ -223,7 +228,7 @@ config_mgr_add_format(config_mgr_t *mgr, static inline config_suite_t ** config_mgr_get_suite_ptr(const config_mgr_t *mgr, void *toplevel) { - if (mgr->toplevel->config_suite_offset < 0) + if (! mgr->toplevel->has_config_suite) return NULL; return STRUCT_VAR_P(toplevel, mgr->toplevel->config_suite_offset); } diff --git a/src/lib/crypt_ops/crypto_init.c b/src/lib/crypt_ops/crypto_init.c index 4b08456197..fbd4da4704 100644 --- a/src/lib/crypt_ops/crypto_init.c +++ b/src/lib/crypt_ops/crypto_init.c @@ -293,7 +293,6 @@ static const config_format_t crypto_options_fmt = { offsetof(crypto_options_t, magic) }, .vars = crypto_options_t_vars, .validate_fn = crypto_options_validate, - .config_suite_offset = -1, }; /** diff --git a/src/test/test_confmgr.c b/src/test/test_confmgr.c index 375a513c07..b59bd8c6a0 100644 --- a/src/test/test_confmgr.c +++ b/src/test/test_confmgr.c @@ -193,6 +193,7 @@ static const config_format_t pasture_fmt = { offsetof(pasture_cfg_t, magic) }, .vars = pasture_vars, + .has_config_suite = true, .config_suite_offset = offsetof(pasture_cfg_t, subobjs), .legacy_validate_fn = legacy_validate_pasture, }; @@ -205,7 +206,6 @@ static const config_format_t llama_fmt = { offsetof(llama_cfg_t, magic) }, .vars = llama_vars, - .config_suite_offset = -1, .deprecations = llama_deprecations, .abbrevs = llama_abbrevs, .clear_fn = clear_llama_cfg, @@ -221,7 +221,6 @@ static const config_format_t alpaca_fmt = { offsetof(alpaca_cfg_t, magic) }, .vars = alpaca_vars, - .config_suite_offset = -1, .deprecations = alpaca_deprecations, .pre_normalize_fn = pre_normalize_alpaca, .check_transition_fn = check_transition_alpaca, diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 39e2de866c..3e122a5129 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -129,7 +129,6 @@ static const config_format_t test_fmt = { .deprecations = test_deprecation_notes, .vars = test_vars, .legacy_validate_fn = test_validate_cb, - .config_suite_offset = -1, }; /* Make sure that config_init sets everything to the right defaults. */ @@ -824,7 +823,6 @@ static config_format_t etest_fmt = { .vars = test_vars, .legacy_validate_fn = test_validate_cb, .extra = &extra, - .config_suite_offset = -1, }; /* Try out the feature where we can store unrecognized lines and dump them From 3afbb29bee060b191e10aaec134a819047c3cf5e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 5 Nov 2019 10:24:34 -0500 Subject: [PATCH 14/19] subsysmgr: use IDX_NONE is an exception value, not -1. --- src/app/main/subsysmgr.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/app/main/subsysmgr.c b/src/app/main/subsysmgr.c index 4189519cad..8be4a7d75c 100644 --- a/src/app/main/subsysmgr.c +++ b/src/app/main/subsysmgr.c @@ -33,15 +33,19 @@ **/ static bool subsystem_array_validated = false; +/** Index value indicating that a subsystem has no options/state object, and + * so that object does not have an index. */ +#define IDX_NONE (-1) + /** * Runtime status of a single subsystem. **/ typedef struct subsys_status_t { /** True if the given subsystem is initialized. */ bool initialized; - /** Index for this subsystem's options object, or -1 for none. */ + /** Index for this subsystem's options object, or IDX_NONE for none. */ int options_idx; - /** Index for this subsystem's state object, or -1 for none. */ + /** Index for this subsystem's state object, or IDX_NONE for none. */ int state_idx; } subsys_status_t; @@ -62,8 +66,8 @@ subsys_status_clear(subsys_status_t *status) return; memset(status, 0, sizeof(*status)); status->initialized = false; - status->state_idx = -1; - status->options_idx = -1; + status->state_idx = IDX_NONE; + status->options_idx = IDX_NONE; } /** From 280a9a476031db7c5c5923b0ad9b23e475abeae9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 5 Nov 2019 11:58:31 -0500 Subject: [PATCH 15/19] Move netstatus (mainloop) state fields into mainloop's state. --- src/app/config/or_state_st.h | 7 ---- src/app/config/statefile.c | 7 +--- src/app/config/statefile.h | 2 ++ src/core/include.am | 2 ++ src/core/mainloop/.may_include | 4 ++- src/core/mainloop/mainloop_state.inc | 19 ++++++++++ src/core/mainloop/mainloop_state_st.h | 23 ++++++++++++ src/core/mainloop/mainloop_sys.c | 52 +++++++++++++++++++++++++++ src/core/mainloop/netstatus.c | 5 +-- src/core/mainloop/netstatus.h | 7 ++-- src/test/test_mainloop.c | 20 ++++++++--- 11 files changed, 126 insertions(+), 22 deletions(-) create mode 100644 src/core/mainloop/mainloop_state.inc create mode 100644 src/core/mainloop/mainloop_state_st.h diff --git a/src/app/config/or_state_st.h b/src/app/config/or_state_st.h index 27cc936c7d..6bfad3edb5 100644 --- a/src/app/config/or_state_st.h +++ b/src/app/config/or_state_st.h @@ -89,13 +89,6 @@ struct or_state_t { /** When did we last rotate our onion key? "0" for 'no idea'. */ time_t LastRotatedOnionKey; - /** Number of minutes since the last user-initiated request (as defined by - * the dormant net-status system.) Set to zero if we are dormant. */ - int MinutesSinceUserActivity; - /** True if we were dormant when we last wrote the file; false if we - * weren't. "auto" on initial startup. */ - int Dormant; - /** * State objects for individual modules. * diff --git a/src/app/config/statefile.c b/src/app/config/statefile.c index 3e793ff402..af64dd47c8 100644 --- a/src/app/config/statefile.c +++ b/src/app/config/statefile.c @@ -132,9 +132,6 @@ static const config_var_t state_vars_[] = { VAR("CircuitBuildTimeBin", LINELIST_S, BuildtimeHistogram, NULL), VAR("BuildtimeHistogram", LINELIST_V, BuildtimeHistogram, NULL), - V(MinutesSinceUserActivity, POSINT, NULL), - V(Dormant, AUTOBOOL, "auto"), - END_OF_CONFIG_VARS }; @@ -177,7 +174,7 @@ static const config_format_t state_format = { static config_mgr_t *state_mgr = NULL; /** Return the configuration manager for state-file objects. */ -static const config_mgr_t * +STATIC const config_mgr_t * get_state_mgr(void) { if (PREDICT_UNLIKELY(state_mgr == NULL)) { @@ -334,7 +331,6 @@ or_state_set(or_state_t *new_state) get_circuit_build_times_mutable(),global_state) < 0) { ret = -1; } - netstatus_load_from_state(global_state, time(NULL)); return ret; } @@ -527,7 +523,6 @@ or_state_save(time_t now) entry_guards_update_state(global_state); rep_hist_update_state(global_state); circuit_build_times_update_state(get_circuit_build_times(), global_state); - netstatus_flush_to_state(global_state, now); if (accounting_is_enabled(get_options())) accounting_run_housekeeping(now); diff --git a/src/app/config/statefile.h b/src/app/config/statefile.h index 515c90a52f..60171f8d13 100644 --- a/src/app/config/statefile.h +++ b/src/app/config/statefile.h @@ -31,6 +31,8 @@ STATIC struct config_line_t *get_transport_in_state_by_name( STATIC void or_state_free_(or_state_t *state); #define or_state_free(st) FREE_AND_NULL(or_state_t, or_state_free_, (st)) STATIC or_state_t *or_state_new(void); +struct config_mgr_t; +STATIC const struct config_mgr_t *get_state_mgr(void); #endif /* defined(STATEFILE_PRIVATE) */ #endif /* !defined(TOR_STATEFILE_H) */ diff --git a/src/core/include.am b/src/core/include.am index a69914619e..193b10a1cc 100644 --- a/src/core/include.am +++ b/src/core/include.am @@ -245,6 +245,8 @@ noinst_HEADERS += \ src/core/mainloop/cpuworker.h \ src/core/mainloop/mainloop.h \ src/core/mainloop/mainloop_pubsub.h \ + src/core/mainloop/mainloop_state.inc \ + src/core/mainloop/mainloop_state_st.h \ src/core/mainloop/mainloop_sys.h \ src/core/mainloop/netstatus.h \ src/core/mainloop/periodic.h \ diff --git a/src/core/mainloop/.may_include b/src/core/mainloop/.may_include index 79d6a130a4..580e6d0a8a 100644 --- a/src/core/mainloop/.may_include +++ b/src/core/mainloop/.may_include @@ -2,6 +2,7 @@ orconfig.h +lib/conf/*.h lib/container/*.h lib/dispatch/*.h lib/evloop/*.h @@ -17,4 +18,5 @@ lib/geoip/*.h lib/sandbox/*.h lib/compress/*.h -core/mainloop/*.h \ No newline at end of file +core/mainloop/*.h +core/mainloop/*.inc \ No newline at end of file diff --git a/src/core/mainloop/mainloop_state.inc b/src/core/mainloop/mainloop_state.inc new file mode 100644 index 0000000000..34a37caaa2 --- /dev/null +++ b/src/core/mainloop/mainloop_state.inc @@ -0,0 +1,19 @@ + +/** + * @file mainloop_state.inc + * @brief Declare configuration options for the crypto_ops module. + **/ + +/** Holds state for the mainloop, corresponding to part of the state + * file in Tor's DataDirectory. */ +BEGIN_CONF_STRUCT(mainloop_state_t) + +/** Number of minutes since the last user-initiated request (as defined by + * the dormant net-status system.) Set to zero if we are dormant. */ +CONF_VAR(MinutesSinceUserActivity, POSINT, 0, NULL) + +/** True if we were dormant when we last wrote the file; false if we + * weren't. "auto" on initial startup. */ +CONF_VAR(Dormant, AUTOBOOL, 0, "auto") + +END_CONF_STRUCT(mainloop_state_t) diff --git a/src/core/mainloop/mainloop_state_st.h b/src/core/mainloop/mainloop_state_st.h new file mode 100644 index 0000000000..44c816fbaf --- /dev/null +++ b/src/core/mainloop/mainloop_state_st.h @@ -0,0 +1,23 @@ +/* Copyright (c) 2001 Matej Pfajfar. + * Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2019, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * @file mainloop_state_st.h + * @brief Declare a state structure for mainloop-relevant fields + **/ + +#ifndef TOR_CORE_MAINLOOP_MAINLOOP_STATE_ST_H +#define TOR_CORE_MAINLOOP_MAINLOOP_STATE_ST_H + +#include "lib/conf/confdecl.h" + +#define CONF_CONTEXT STRUCT +#include "core/mainloop/mainloop_state.inc" +#undef CONF_CONTEXT + +typedef struct mainloop_state_t mainloop_state_t; + +#endif /* !defined(TOR_CORE_MAINLOOP_MAINLOOP_STATE_ST_H) */ diff --git a/src/core/mainloop/mainloop_sys.c b/src/core/mainloop/mainloop_sys.c index f14ecb261b..7d763866dc 100644 --- a/src/core/mainloop/mainloop_sys.c +++ b/src/core/mainloop/mainloop_sys.c @@ -12,6 +12,10 @@ #include "core/or/or.h" #include "core/mainloop/mainloop_sys.h" #include "core/mainloop/mainloop.h" +#include "core/mainloop/mainloop_state_st.h" +#include "core/mainloop/netstatus.h" +#include "lib/conf/conftypes.h" +#include "lib/conf/confdecl.h" #include "lib/subsys/subsys.h" @@ -28,10 +32,58 @@ subsys_mainloop_shutdown(void) tor_mainloop_free_all(); } +/** Declare a list of state variables for mainloop state. */ +#define CONF_CONTEXT TABLE +#include "core/mainloop/mainloop_state.inc" +#undef CONF_CONTEXT + +/** Magic number for mainloop state objects */ +#define MAINLOOP_STATE_MAGIC 0x59455449 + +/** + * Format object for mainloop state. + **/ +static config_format_t mainloop_state_fmt = { + .size = sizeof(mainloop_state_t), + .magic = { "mainloop_state", + MAINLOOP_STATE_MAGIC, + offsetof(mainloop_state_t, magic) + }, + .vars = mainloop_state_t_vars, +}; + +/** + */ +static int +mainloop_set_state(void *arg) +{ + const mainloop_state_t *state = arg; + tor_assert(state->magic == MAINLOOP_STATE_MAGIC); + + netstatus_load_from_state(state, approx_time()); + + return 0; +} + +static int +mainloop_flush_state(void *arg) +{ + mainloop_state_t *state = arg; + tor_assert(state->magic == MAINLOOP_STATE_MAGIC); + + netstatus_flush_to_state(state, approx_time()); + + return 0; +} + const struct subsys_fns_t sys_mainloop = { .name = "mainloop", .supported = true, .level = 5, .initialize = subsys_mainloop_initialize, .shutdown = subsys_mainloop_shutdown, + + .state_format = &mainloop_state_fmt, + .set_state = mainloop_set_state, + .flush_state = mainloop_flush_state, }; diff --git a/src/core/mainloop/netstatus.c b/src/core/mainloop/netstatus.c index c34e613d1f..a7a1927d83 100644 --- a/src/core/mainloop/netstatus.c +++ b/src/core/mainloop/netstatus.c @@ -12,6 +12,7 @@ #include "core/or/or.h" #include "core/mainloop/netstatus.h" #include "core/mainloop/mainloop.h" +#include "core/mainloop/mainloop_state_st.h" #include "app/config/config.h" #include "feature/hibernate/hibernate.h" @@ -115,7 +116,7 @@ is_participating_on_network(void) * Update 'state' with the last time at which we were active on the network. **/ void -netstatus_flush_to_state(or_state_t *state, time_t now) +netstatus_flush_to_state(mainloop_state_t *state, time_t now) { state->Dormant = ! participating_on_network; if (participating_on_network) { @@ -130,7 +131,7 @@ netstatus_flush_to_state(or_state_t *state, time_t now) * Update our current view of network participation from an or_state_t object. **/ void -netstatus_load_from_state(const or_state_t *state, time_t now) +netstatus_load_from_state(const mainloop_state_t *state, time_t now) { time_t last_activity; if (state->Dormant == -1) { // Initial setup. diff --git a/src/core/mainloop/netstatus.h b/src/core/mainloop/netstatus.h index ce3d2e23f9..62fd77b42e 100644 --- a/src/core/mainloop/netstatus.h +++ b/src/core/mainloop/netstatus.h @@ -22,8 +22,11 @@ time_t get_last_user_activity_time(void); void set_network_participation(bool participation); bool is_participating_on_network(void); -void netstatus_flush_to_state(or_state_t *state, time_t now); -void netstatus_load_from_state(const or_state_t *state, time_t now); +struct mainloop_state_t; + +void netstatus_flush_to_state(struct mainloop_state_t *state, time_t now); +void netstatus_load_from_state(const struct mainloop_state_t *state, + time_t now); void netstatus_note_clock_jumped(time_t seconds_diff); #endif /* !defined(TOR_NETSTATUS_H) */ diff --git a/src/test/test_mainloop.c b/src/test/test_mainloop.c index ed6b8a9b66..e8225db8e0 100644 --- a/src/test/test_mainloop.c +++ b/src/test/test_mainloop.c @@ -13,9 +13,13 @@ #include "test/test.h" #include "test/log_test_helpers.h" +#include "lib/confmgt/confmgt.h" + #include "core/or/or.h" #include "core/mainloop/connection.h" #include "core/mainloop/mainloop.h" +#include "core/mainloop/mainloop_state_st.h" +#include "core/mainloop/mainloop_sys.h" #include "core/mainloop/netstatus.h" #include "feature/hs/hs_service.h" @@ -24,6 +28,8 @@ #include "app/config/statefile.h" #include "app/config/or_state_st.h" +#include "app/main/subsysmgr.h" + static const uint64_t BILLION = 1000000000; static void @@ -287,7 +293,13 @@ static void test_mainloop_dormant_load_state(void *arg) { (void)arg; - or_state_t *state = or_state_new(); + or_state_t *or_state = or_state_new(); + mainloop_state_t *state; + { + int idx = subsystems_get_state_idx(&sys_mainloop); + tor_assert(idx >= 0); + state = config_mgr_get_obj_mutable(get_state_mgr(), or_state, idx); + } const time_t start = 1543956575; reset_user_activity(0); @@ -326,14 +338,14 @@ test_mainloop_dormant_load_state(void *arg) tt_i64_op(get_last_user_activity_time(), OP_EQ, start); done: - or_state_free(state); + or_state_free(or_state); } static void test_mainloop_dormant_save_state(void *arg) { (void)arg; - or_state_t *state = or_state_new(); + mainloop_state_t *state = tor_malloc_zero(sizeof(mainloop_state_t)); const time_t start = 1543956575; // Can we save a non-dormant state correctly? @@ -352,7 +364,7 @@ test_mainloop_dormant_save_state(void *arg) tt_int_op(state->MinutesSinceUserActivity, OP_EQ, 0); done: - or_state_free(state); + tor_free(state); } #define MAINLOOP_TEST(name) \ From 27a4438805dcac7fa46a2c47f0b247c7f8d3668e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Nov 2019 11:02:37 -0500 Subject: [PATCH 16/19] conf_examples test for crypto_accel --- src/test/conf_examples/crypto_accel/expected | 2 ++ src/test/conf_examples/crypto_accel/torrc | 3 +++ 2 files changed, 5 insertions(+) create mode 100644 src/test/conf_examples/crypto_accel/expected create mode 100644 src/test/conf_examples/crypto_accel/torrc diff --git a/src/test/conf_examples/crypto_accel/expected b/src/test/conf_examples/crypto_accel/expected new file mode 100644 index 0000000000..ea80ca19dc --- /dev/null +++ b/src/test/conf_examples/crypto_accel/expected @@ -0,0 +1,2 @@ +AccelName nonexistent_chartreuse_accelerator +HardwareAccel 1 diff --git a/src/test/conf_examples/crypto_accel/torrc b/src/test/conf_examples/crypto_accel/torrc new file mode 100644 index 0000000000..9ca18903b7 --- /dev/null +++ b/src/test/conf_examples/crypto_accel/torrc @@ -0,0 +1,3 @@ + +AccelName nonexistent_chartreuse_accelerator +HardwareAccel 1 From 32a2f96f82eaadafd4faf2c6184093e441661a4e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Nov 2019 11:12:09 -0500 Subject: [PATCH 17/19] Add the ability to fail when a hardware accelerator is missing. Closes ticket 32406. --- changes/ticket32406 | 4 ++++ doc/tor.1.txt | 3 +++ src/lib/crypt_ops/crypto_openssl_mgt.c | 20 +++++++++++++++++--- 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 changes/ticket32406 diff --git a/changes/ticket32406 b/changes/ticket32406 new file mode 100644 index 0000000000..c0c60207c0 --- /dev/null +++ b/changes/ticket32406 @@ -0,0 +1,4 @@ + o Minor features (configuration): + - If the configured hardware crypto accelerator in AccelName + is prefixed with "!", Tor now exits when it cannot be found. + Closes ticket 32406. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 5e3953e332..a3c05961e3 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -843,6 +843,9 @@ GENERAL OPTIONS engine of this name. This must be used for any dynamic hardware engine. Names can be verified with the openssl engine command. Can not be changed while tor is running. + + + If the engine name is prefixed with a "!", then Tor will exit if the + engine cannot be loaded. [[AccelDir]] **AccelDir** __DIR__:: Specify this option if using dynamic hardware acceleration and the engine diff --git a/src/lib/crypt_ops/crypto_openssl_mgt.c b/src/lib/crypt_ops/crypto_openssl_mgt.c index 2fbebd87e0..d7a3e0692b 100644 --- a/src/lib/crypt_ops/crypto_openssl_mgt.c +++ b/src/lib/crypt_ops/crypto_openssl_mgt.c @@ -275,8 +275,14 @@ log_engine(const char *fn, ENGINE *e) } #endif /* !defined(DISABLE_ENGINES) */ -/** Initialize engines for openssl (if enabled). */ -static void +/** Initialize engines for openssl (if enabled). Load all the built-in + * engines, along with the one called accelName (which may be NULL). + * If accelName is prefixed with "!", then it is required: return -1 + * if it can't be loaded. Otherwise return 0. + * + * If accelDir is not NULL, it is the path from which the engine should + * be loaded. */ +static int crypto_openssl_init_engines(const char *accelName, const char *accelDir) { @@ -284,6 +290,7 @@ crypto_openssl_init_engines(const char *accelName, (void)accelName; (void)accelDir; log_warn(LD_CRYPTO, "No OpenSSL hardware acceleration support enabled."); + return 0; #else ENGINE *e = NULL; @@ -292,6 +299,9 @@ crypto_openssl_init_engines(const char *accelName, ENGINE_register_all_complete(); if (accelName) { + const bool required = accelName[0] == '!'; + if (required) + ++accelName; if (accelDir) { log_info(LD_CRYPTO, "Trying to load dynamic OpenSSL engine \"%s\"" " via path \"%s\".", accelName, accelDir); @@ -304,6 +314,8 @@ crypto_openssl_init_engines(const char *accelName, if (!e) { log_warn(LD_CRYPTO, "Unable to load dynamic OpenSSL engine \"%s\".", accelName); + if (required) + return -1; } else { log_info(LD_CRYPTO, "Loaded dynamic OpenSSL engine \"%s\".", accelName); @@ -340,6 +352,7 @@ crypto_openssl_init_engines(const char *accelName, #ifdef NID_aes_256_gcm log_engine("AES-256-GCM", ENGINE_get_cipher_engine(NID_aes_256_gcm)); #endif + return 0; #endif /* defined(DISABLE_ENGINES) */ } @@ -350,7 +363,8 @@ crypto_openssl_late_init(int useAccel, const char *accelName, const char *accelDir) { if (useAccel > 0) { - crypto_openssl_init_engines(accelName, accelDir); + if (crypto_openssl_init_engines(accelName, accelDir) < 0) + return -1; } else { log_info(LD_CRYPTO, "NOT using OpenSSL engine support."); } From c550990400cbab450eaead69f6dd5261b89b484f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 6 Nov 2019 11:19:27 -0500 Subject: [PATCH 18/19] Add a test for 32406 (AccelName with "!"). This also lets us test a failing set_options(). --- src/test/conf_examples/crypto_accel_req/error | 1 + src/test/conf_examples/crypto_accel_req/expected_nss | 2 ++ src/test/conf_examples/crypto_accel_req/torrc | 3 +++ 3 files changed, 6 insertions(+) create mode 100644 src/test/conf_examples/crypto_accel_req/error create mode 100644 src/test/conf_examples/crypto_accel_req/expected_nss create mode 100644 src/test/conf_examples/crypto_accel_req/torrc diff --git a/src/test/conf_examples/crypto_accel_req/error b/src/test/conf_examples/crypto_accel_req/error new file mode 100644 index 0000000000..7fab0c8dad --- /dev/null +++ b/src/test/conf_examples/crypto_accel_req/error @@ -0,0 +1 @@ +Unable to load dynamic OpenSSL engine "nonexistent_chartreuse_accelerator" diff --git a/src/test/conf_examples/crypto_accel_req/expected_nss b/src/test/conf_examples/crypto_accel_req/expected_nss new file mode 100644 index 0000000000..f3e172f640 --- /dev/null +++ b/src/test/conf_examples/crypto_accel_req/expected_nss @@ -0,0 +1,2 @@ +AccelName !nonexistent_chartreuse_accelerator +HardwareAccel 1 diff --git a/src/test/conf_examples/crypto_accel_req/torrc b/src/test/conf_examples/crypto_accel_req/torrc new file mode 100644 index 0000000000..981d9116fc --- /dev/null +++ b/src/test/conf_examples/crypto_accel_req/torrc @@ -0,0 +1,3 @@ + +AccelName !nonexistent_chartreuse_accelerator +HardwareAccel 1 From de7053b8967db64ae2a871d11b12afbcb9b1f8a6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 7 Nov 2019 07:42:58 -0500 Subject: [PATCH 19/19] Adjustments to 32406 per review When we are failing because of a lack of a _required_ engine, note that the engine was "required". When engines are disabled, any required engine should cause a failure. --- src/lib/crypt_ops/crypto_openssl_mgt.c | 8 +++++++- src/test/conf_examples/crypto_accel_req/error | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/lib/crypt_ops/crypto_openssl_mgt.c b/src/lib/crypt_ops/crypto_openssl_mgt.c index d7a3e0692b..f028422acb 100644 --- a/src/lib/crypt_ops/crypto_openssl_mgt.c +++ b/src/lib/crypt_ops/crypto_openssl_mgt.c @@ -290,6 +290,11 @@ crypto_openssl_init_engines(const char *accelName, (void)accelName; (void)accelDir; log_warn(LD_CRYPTO, "No OpenSSL hardware acceleration support enabled."); + if (accelName && accelName[0] == '!') { + log_warn(LD_CRYPTO, "Unable to load required dynamic OpenSSL engine " + "\"%s\".", accelName+1); + return -1; + } return 0; #else ENGINE *e = NULL; @@ -312,7 +317,8 @@ crypto_openssl_init_engines(const char *accelName, e = ENGINE_by_id(accelName); } if (!e) { - log_warn(LD_CRYPTO, "Unable to load dynamic OpenSSL engine \"%s\".", + log_warn(LD_CRYPTO, "Unable to load %sdynamic OpenSSL engine \"%s\".", + required?"required ":"", accelName); if (required) return -1; diff --git a/src/test/conf_examples/crypto_accel_req/error b/src/test/conf_examples/crypto_accel_req/error index 7fab0c8dad..e12e002915 100644 --- a/src/test/conf_examples/crypto_accel_req/error +++ b/src/test/conf_examples/crypto_accel_req/error @@ -1 +1 @@ -Unable to load dynamic OpenSSL engine "nonexistent_chartreuse_accelerator" +Unable to load required dynamic OpenSSL engine "nonexistent_chartreuse_accelerator"