r17499@catbus: nickm | 2008-01-07 13:39:46 -0500

 Bugfix on fix for 557: Make values containing special characters work right with getconf, setconf, and saveconf.  Document this in control-spec.txt


svn:r13056

ChangeLog

@@ -36,6 +36,9 @@ Changes in version 0.2.0.16-alpha - 2008-01-??
 
   o Minor features (controller):
     - Get NS events working again.  (Patch from tup)
+    - The GETCONF command now escapes and quotes configuration values
+      that don't otherwise fit into the torrc file.
+    - The SETCONF command now handles quoted values correctly.
 
   o Minor features (other):
     - Add hidden services and DNSPorts to the list of things that make
@@ -43,6 +46,7 @@ Changes in version 0.2.0.16-alpha - 2008-01-??
       no ports from a fatal error to a warning; we might change it
       back if this turns out to confuse anybody.  (Fixes bug 579.)
 
+
 Changes in version 0.2.0.15-alpha - 2007-12-25
   o Major bugfixes:
     - Fix several remotely triggerable asserts based on DirPort requests

doc/spec/control-spec.txt

@@ -139,7 +139,8 @@ $Id$
 
   Change the value of one or more configuration variables.  The syntax is:
 
-    "SETCONF" 1*(SP keyword ["=" String]) CRLF
+    "SETCONF" 1*(SP keyword ["=" value]) CRLF
+    value = String / QuotedString
 
   Tor behaves as though it had just read each of the key-value pairs
   from its configuration file.  Keywords with no corresponding values have
@@ -184,6 +185,10 @@ $Id$
   empty string, Tor may reply with a reply line of the form:
       250 keyword
 
+  Value may be a raw value or a quoted string.  Tor will try to use
+  unquoted values except when the value could be misinterpreted through
+  not being quoted.
+
   If some of the listed keywords can't be found, Tor replies with a
   "552 unknown configuration keyword" message.
 

src/or/config.c

@@ -627,7 +627,8 @@ static int parse_log_severity_range(const char *range, int *min_out,
 static int validate_data_directory(or_options_t *options);
 static int write_configuration_file(const char *fname, or_options_t *options);
 static config_line_t *get_assigned_option(config_format_t *fmt,
-                                     or_options_t *options, const char *key);
+                                      or_options_t *options, const char *key,
+                                      int escape_val);
 static void config_init(config_format_t *fmt, void *options);
 static int or_state_validate(or_state_t *old_options, or_state_t *options,
                              int from_setconf, char **msg);
@@ -1686,7 +1687,33 @@ option_get_canonical_name(const char *key)
 config_line_t *
 option_get_assignment(or_options_t *options, const char *key)
 {
-  return get_assigned_option(&options_format, options, key);
+  return get_assigned_option(&options_format, options, key, 1);
+}
+
+/** Return true iff value needs to be quoted and escaped to be used in
+ * a configuration file. */
+static int
+config_value_needs_escape(const char *value)
+{
+  if (*value == '\"')
+    return 1;
+  while (*value) {
+    switch (*value)
+    {
+    case '\r':
+    case '\n':
+    case '#':
+      /* Note: quotes and backspaces need special handling when we are using
+       * quotes, not otherwise, so they don't trigger escaping on their
+       * own. */
+      return 1;
+    default:
+      if (!TOR_ISPRINT(*value))
+        return 1;
+    }
+    ++value;
+  }
+  return 0;
 }
 
 /** Return a newly allocated deep copy of the lines in <b>inp</b>. */
@@ -1707,10 +1734,12 @@ config_lines_dup(const config_line_t *inp)
 }
 
 /** Return newly allocated line or lines corresponding to <b>key</b> in the
- * configuration <b>options</b>.  Return NULL if no such key exists. */
+ * configuration <b>options</b>.  If <b>escape_val</b> is true and a
+ * value needs to be quoted before it's put in a config file, quote and
+ * escape that value. Return NULL if no such key exists. */
 static config_line_t *
 get_assigned_option(config_format_t *fmt, or_options_t *options,
-                    const char *key)
+                    const char *key, int escape_val)
 /* XXXX argument is options, but fmt is provided. Inconsistent. */
 {
   config_var_t *var;
@@ -1749,6 +1778,7 @@ get_assigned_option(config_format_t *fmt, or_options_t *options,
         tor_free(result->key);
         tor_free(result);
       }
+      escape_val = 0; /* Can't need escape. */
       break;
     case CONFIG_TYPE_INTERVAL:
     case CONFIG_TYPE_UINT:
@@ -1756,18 +1786,22 @@ get_assigned_option(config_format_t *fmt, or_options_t *options,
        * needs to be an int. Not, say, a uint16_t or char. */
       tor_snprintf(buf, sizeof(buf), "%d", *(int*)value);
       result->value = tor_strdup(buf);
+      escape_val = 0; /* Can't need escape. */
       break;
     case CONFIG_TYPE_MEMUNIT:
       tor_snprintf(buf, sizeof(buf), U64_FORMAT,
                    U64_PRINTF_ARG(*(uint64_t*)value));
       result->value = tor_strdup(buf);
+      escape_val = 0; /* Can't need escape. */
       break;
     case CONFIG_TYPE_DOUBLE:
       tor_snprintf(buf, sizeof(buf), "%f", *(double*)value);
       result->value = tor_strdup(buf);
+      escape_val = 0; /* Can't need escape. */
       break;
     case CONFIG_TYPE_BOOL:
       result->value = tor_strdup(*(int*)value ? "1" : "0");
+      escape_val = 0; /* Can't need escape. */
       break;
     case CONFIG_TYPE_CSV:
       if (*(smartlist_t**)value)
@@ -1793,7 +1827,8 @@ get_assigned_option(config_format_t *fmt, or_options_t *options,
     case CONFIG_TYPE_LINELIST_V:
       tor_free(result->key);
       tor_free(result);
-      return config_lines_dup(*(const config_line_t**)value);
+      result = config_lines_dup(*(const config_line_t**)value);
+      break;
     default:
       tor_free(result->key);
       tor_free(result);
@@ -1802,6 +1837,17 @@ get_assigned_option(config_format_t *fmt, or_options_t *options,
       return NULL;
     }
 
+  if (escape_val) {
+    config_line_t *line;
+    for (line = result; line; line = line->next) {
+      if (line->value && config_value_needs_escape(line->value)) {
+        char *newval = esc_for_log(line->value);
+        tor_free(line->value);
+        line->value = newval;
+      }
+    }
+  }
+
   return result;
 }
 
@@ -2319,8 +2365,8 @@ option_is_same(config_format_t *fmt,
   CHECK(fmt, o1);
   CHECK(fmt, o2);
 
-  c1 = get_assigned_option(fmt, o1, name);
-  c2 = get_assigned_option(fmt, o2, name);
+  c1 = get_assigned_option(fmt, o1, name, 0);
+  c2 = get_assigned_option(fmt, o2, name, 0);
   r = config_lines_eq(c1, c2);
   config_free_lines(c1);
   config_free_lines(c2);
@@ -2341,7 +2387,7 @@ options_dup(config_format_t *fmt, or_options_t *old)
       continue;
     if (fmt->vars[i].type == CONFIG_TYPE_OBSOLETE)
       continue;
-    line = get_assigned_option(fmt, old, fmt->vars[i].name);
+    line = get_assigned_option(fmt, old, fmt->vars[i].name, 0);
     if (line) {
       char *msg = NULL;
       if (config_assign(fmt, newopts, line, 0, 0, &msg) < 0) {
@@ -2430,7 +2476,7 @@ config_dump(config_format_t *fmt, void *options, int minimal,
       comment_option = 1;
 
     desc = config_find_description(fmt, fmt->vars[i].name);
-    line = assigned = get_assigned_option(fmt, options, fmt->vars[i].name);
+    line = assigned = get_assigned_option(fmt, options, fmt->vars[i].name, 1);
 
     if (line && desc) {
       /* Only dump the description if there's something to describe. */

src/or/control.c

@@ -364,6 +364,44 @@ read_escaped_data(const char *data, size_t len, char **out)
   return outp - *out;
 }
 
+/** DOCDOC */
+static const char *
+extract_escaped_string(const char *start, size_t in_len_max,
+                       char **out, size_t *out_len)
+{
+  const char *cp, *end;
+  size_t len=0;
+
+  if (*start != '\"')
+    return NULL;
+
+  cp = start+1;
+  end = start+in_len_max;
+
+  /* Calculate length. */
+  while (1) {
+    if (cp >= end)
+      return NULL;
+    else if (*cp == '\\') {
+      if (++cp == end)
+        return NULL; /* Can't escape EOS. */
+      ++cp;
+      ++len;
+    } else if (*cp == '\"') {
+      break;
+    } else {
+      ++cp;
+      ++len;
+    }
+  }
+  end = cp;
+
+  *out_len = end-start+1;
+  *out = tor_strndup(start, *out_len);
+
+  return end+1;
+}
+
 /** Given a pointer to a string starting at <b>start</b> containing
  * <b>in_len_max</b> characters, decode a string beginning with one double
  * quote, containing any number of non-quote characters or characters escaped
@@ -372,6 +410,7 @@ read_escaped_data(const char *data, size_t len, char **out)
  * store its length in <b>out_len</b>.  On success, return a pointer to the
  * character immediately following the escaped string.  On failure, return
  * NULL. */
+/* XXXX020 fold into extract_escaped_string */
 static const char *
 get_escaped_string(const char *start, size_t in_len_max,
                    char **out, size_t *out_len)
@@ -659,8 +698,8 @@ control_setconf_helper(control_connection_t *conn, uint32_t len, char *body,
         val = tor_strndup(val_start, body-val_start);
         val_len = strlen(val);
       } else {
-        body = (char*)get_escaped_string(body, (len - (body-start)),
-                                         &val, &val_len);
+        body = (char*)extract_escaped_string(body, (len - (body-start)),
+                                             &val, &val_len);
         if (!body) {
           connection_write_str_to_buf("551 Couldn't parse string\r\n", conn);
           SMARTLIST_FOREACH(entries, char *, cp, tor_free(cp));