[tor-commits] [tor/master] Add comments to try to prevent recurrence of #31495.

[teor at torproject.org]

commit 39640728c332980daf7ca639827735a1c359669a
Author: Nick Mathewson <nickm at torproject.org>
Date:   Tue Oct 1 09:42:10 2019 -0400

    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.
---
 src/feature/nodelist/routerset.c  | 7 +++++++
 src/lib/confmgt/type_defs.c       | 4 ++++
 src/lib/confmgt/var_type_def_st.h | 6 ++++++
 3 files changed, 17 insertions(+)

diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c
index 73c2b1b1d..9a205d39b 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 62c12fcdd..ed930fb02 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 f1131ff11..2bf3d37ca 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 {
   /**