diff options
| author | Nick Mathewson <nickm@torproject.org> | 2019-10-01 09:42:10 -0400 |
|---|---|---|
| committer | Nick Mathewson <nickm@torproject.org> | 2019-10-01 09:45:12 -0400 |
| commit | 39640728c332980daf7ca639827735a1c359669a (patch) | |
| tree | bc8104a221760d32ec7d7c48b958d5d349cceb6b | |
| parent | 53116ca0b71898b46a6d678407fb1b03c2ab8a93 (diff) | |
| download | tor-39640728c332980daf7ca639827735a1c359669a.tar.gz tor-39640728c332980daf7ca639827735a1c359669a.zip | |
Add comments to try to prevent recurrence of #31495.
There is a bad design choice in two of our configuration types,
where the empty string encodes a value that is not the same as the
default value. This design choice, plus an implementation mistake,
meant that config_dup() did not preserve the value of routerset_t,
and thereby caused bug #31495.
This comment-only patch documents the two types with the problem,
and suggests that implementors try to avoid it in the future.
Closes ticket 31907.
| -rw-r--r-- | src/feature/nodelist/routerset.c | 7 | ||||
| -rw-r--r-- | src/lib/confmgt/type_defs.c | 4 | ||||
| -rw-r--r-- | src/lib/confmgt/var_type_def_st.h | 6 |
3 files changed, 17 insertions, 0 deletions
diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c index 73c2b1b1de..9a205d39b7 100644 --- a/src/feature/nodelist/routerset.c +++ b/src/feature/nodelist/routerset.c @@ -473,6 +473,13 @@ routerset_free_(routerset_t *routerset) * routerset_t** passed as <b>target</b>. On success return 0; on failure * return -1 and store an error message into *<b>errmsg</b>. **/ +/* + * Warning: For this type, the default value (NULL) and "" are sometimes + * considered different values. That is generally risky, and best avoided for + * other types in the future. For cases where we want the default to be "all + * routers" (like EntryNodes) we should add a new routerset value indicating + * "all routers" (see #31908) + */ static int routerset_kv_parse(void *target, const config_line_t *line, char **errmsg, const void *params) diff --git a/src/lib/confmgt/type_defs.c b/src/lib/confmgt/type_defs.c index 62c12fcddd..ed930fb02a 100644 --- a/src/lib/confmgt/type_defs.c +++ b/src/lib/confmgt/type_defs.c @@ -44,6 +44,10 @@ // CONFIG_TYPE_FILENAME // // These two types are the same for now, but they have different names. +// +// Warning: For this type, the default value (NULL) and "" are considered +// different values. That is generally risky, and best avoided for other +// types in the future. ////// static int diff --git a/src/lib/confmgt/var_type_def_st.h b/src/lib/confmgt/var_type_def_st.h index f1131ff116..2bf3d37cae 100644 --- a/src/lib/confmgt/var_type_def_st.h +++ b/src/lib/confmgt/var_type_def_st.h @@ -39,6 +39,12 @@ struct config_line_t; * All functions here take a <b>params</b> argument, whose value * is determined by the type definition. Two types may have the * same functions, but differ only in parameters. + * + * Implementation considerations: If "" encodes a valid value for a type, try + * to make sure that it encodes the same thing as the default value for the + * type (that is, the value that is set by config_clear() or memset(0)). If + * this is not the case, you need to make extra certain that your parse/encode + * implementations preserve the NULL/"" distinction. **/ struct var_type_fns_t { /** |