subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From stef...@apache.org
Subject svn commit: r1688028 - in /subversion/trunk/subversion: libsvn_subr/config.c tests/libsvn_subr/config-test.c tests/libsvn_subr/config-test.cfg
Date Sun, 28 Jun 2015 17:50:20 GMT
Author: stefan2
Date: Sun Jun 28 17:50:20 2015
New Revision: 1688028

URL: http://svn.apache.org/r1688028
Log:
Fix issue #4543:
Recursive configuration value definitions shall not segfault.
They are still unsupported but now result in defined behavior.

To prevent stack overflow or OOM errors, we need to detect cycles in
configuration value definitions.  This patch replaces the boolean
"expanded" state with a enum that allows us to e.g. mark a value as
"being in the process of being expanded", thus enables us to detect
cycles.  We also use it to mark a value as "cyclic" after detection.

Because the public API does not provide any means of error return,
we must implement a defined, deterministic answer for recursive value
definitions.  With this patch, all values that depend on a recursive
definition are normalized to empty strings.  That increases the chance
of users actually noticing their mistake.

* subversion/libsvn_subr/config.c
  (option_state_t): New value resolution state type.
  (cfg_option_t): Replace the binary resolution state.
  (rmex_callback): Update the reset logic.
  (make_string_from_option): Detect and propagate cycles using the
                             refined state machine.
  (expand_option_value): Return whether the expansion was successful.
  (svn_config_create_option): Update constructor.
  (svn_config__is_expanded): Update state check.
  (svn_config_get): Default values need also be checked for references
                    to recursive definitions.
  (svn_config_set): Update initial state.
  (svn_config_dup): Update element access.

* subversion/tests/libsvn_subr/config-test.cfg
  (section1): Add recursive value definitions.

* subversion/tests/libsvn_subr/config-test.c
  (test_expand): Extend to check the recursive definitions as well.

Modified:
    subversion/trunk/subversion/libsvn_subr/config.c
    subversion/trunk/subversion/tests/libsvn_subr/config-test.c
    subversion/trunk/subversion/tests/libsvn_subr/config-test.cfg

Modified: subversion/trunk/subversion/libsvn_subr/config.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/config.c?rev=1688028&r1=1688027&r2=1688028&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/config.c (original)
+++ subversion/trunk/subversion/libsvn_subr/config.c Sun Jun 28 17:50:20 2015
@@ -55,6 +55,27 @@ struct cfg_section_t
 };
 
 
+/* States that a config option value can assume. */
+typedef enum option_state_t
+{
+  /* Value still needs to be expanded.
+     This is the initial state for *all* values. */
+  option_state_needs_expanding,
+
+  /* Value is currently being expanded.
+     This transitional state allows for detecting cyclic dependencies. */
+  option_state_expanding,
+
+  /* Expanded value is available.
+     Values that never needed expanding directly go into that state
+     skipping option_state_expanding. */
+  option_state_expanded,
+
+  /* The value expansion is cyclic which results in "undefined" behavior.
+     This is to return a defined value ("") in that case. */
+  option_state_cyclic
+} option_state_t;
+
 /* Option table entries. */
 typedef struct cfg_option_t cfg_option_t;
 struct cfg_option_t
@@ -71,10 +92,10 @@ struct cfg_option_t
   /* The expanded option value. */
   const char *x_value;
 
-  /* Expansion flag. If this is TRUE, this value has already been expanded.
-     In this case, if x_value is NULL, no expansions were necessary,
-     and value should be used directly. */
-  svn_boolean_t expanded;
+  /* Expansion state. If this is option_state_expanded, VALUE has already
+     been expanded.  In this case, if x_value is NULL, no expansions were
+     necessary, and value should be used directly. */
+  option_state_t state;
 };
 
 
@@ -396,12 +417,13 @@ svn_config_merge(svn_config_t *cfg, cons
 static svn_boolean_t
 rmex_callback(void *baton, cfg_section_t *section, cfg_option_t *option)
 {
-  /* Only clear the `expanded' flag if the value actually contains
+  /* Only reset the expansion state if the value actually contains
      variable expansions. */
-  if (option->expanded && option->x_value != NULL)
+  if (   (option->state == option_state_expanded && option->x_value != NULL)
+      || option->state == option_state_cyclic)
     {
       option->x_value = NULL;
-      option->expanded = FALSE;
+      option->state = option_state_needs_expanding;
     }
 
   return FALSE;
@@ -482,7 +504,7 @@ find_option(svn_config_t *cfg, const cha
 
 
 /* Has a bi-directional dependency with make_string_from_option(). */
-static void
+static svn_boolean_t
 expand_option_value(svn_config_t *cfg, cfg_section_t *section,
                     const char *opt_value, const char **opt_x_valuep,
                     apr_pool_t *x_pool);
@@ -496,7 +518,20 @@ make_string_from_option(const char **val
                         apr_pool_t* x_pool)
 {
   /* Expand the option value if necessary. */
-  if (!opt->expanded)
+  if (   opt->state == option_state_expanding
+      || opt->state == option_state_cyclic)
+    {
+      /* Recursion is not supported.  Since we can't produce an error
+       * nor should we abort the process, the next best thing is to
+       * report the recursive part as an empty string. */
+      *valuep = "";
+
+      /* Go into "value undefined" state. */
+      opt->state = option_state_cyclic;
+
+      return;
+    }
+  else if (opt->state == option_state_needs_expanding)
     {
       /* before attempting to expand an option, check for the placeholder.
        * If none is there, there is no point in calling expand_option_value.
@@ -511,9 +546,16 @@ make_string_from_option(const char **val
 
           tmp_pool = (x_pool ? x_pool : svn_pool_create(cfg->x_pool));
 
-          expand_option_value(cfg, section, opt->value, &opt->x_value, tmp_pool);
-          opt->expanded = TRUE;
+          /* Expand the value. During that process, have the option marked
+           * as "expanding" to detect cycles. */
+          opt->state = option_state_expanding;
+          if (expand_option_value(cfg, section, opt->value, &opt->x_value,
+                                  tmp_pool))
+            opt->state = option_state_expanded;
+          else
+            opt->state = option_state_cyclic;
 
+          /* Ensure the expanded value is allocated in a permanent pool. */
           if (x_pool != cfg->x_pool)
             {
               /* Grab the fully expanded value from tmp_pool before its
@@ -527,7 +569,7 @@ make_string_from_option(const char **val
         }
       else
         {
-          opt->expanded = TRUE;
+          opt->state = option_state_expanded;
         }
     }
 
@@ -549,8 +591,9 @@ make_string_from_option(const char **val
 
 /* Expand OPT_VALUE (which may be NULL) in SECTION into *OPT_X_VALUEP.
    If no variable replacements are done, set *OPT_X_VALUEP to
-   NULL. Allocate from X_POOL. */
-static void
+   NULL.  Return TRUE if the expanded value is defined and FALSE
+   for recursive definitions.  Allocate from X_POOL. */
+static svn_boolean_t
 expand_option_value(svn_config_t *cfg, cfg_section_t *section,
                     const char *opt_value, const char **opt_x_valuep,
                     apr_pool_t *x_pool)
@@ -587,6 +630,18 @@ expand_option_value(svn_config_t *cfg, c
                  should terminate. */
               make_string_from_option(&cstring, cfg, section, x_opt, x_pool);
 
+              /* Values depending on cyclic values must also be marked as
+               * "undefined" because they might themselves form cycles with
+               * the one cycle we just detected.  Due to the early abort of
+               * the recursion, we won't follow and thus detect dependent
+               * cycles anymore.
+               */
+              if (x_opt->state == option_state_cyclic)
+                {
+                  *opt_x_valuep = "";
+                  return FALSE;
+                }
+
               /* Append the plain text preceding the expansion. */
               len = name_start - FMT_START_LEN - copy_from;
               if (buf == NULL)
@@ -625,6 +680,9 @@ expand_option_value(svn_config_t *cfg, c
     }
   else
     *opt_x_valuep = NULL;
+
+  /* Expansion has a well-defined answer. */
+  return TRUE;
 }
 
 static cfg_section_t *
@@ -664,7 +722,7 @@ svn_config_create_option(cfg_option_t **
 
   o->value = apr_pstrdup(pool, value);
   o->x_value = NULL;
-  o->expanded = FALSE;
+  o->state = option_state_needs_expanding;
 
   *opt = o;
 }
@@ -685,7 +743,8 @@ svn_config__is_expanded(svn_config_t *cf
     return FALSE;
 
   /* already expanded? */
-  if (opt->expanded)
+  if (   opt->state == option_state_expanded
+      || opt->state == option_state_cyclic)
     return TRUE;
 
   /* needs expansion? */
@@ -719,8 +778,14 @@ svn_config_get(svn_config_t *cfg, const
           {
             apr_pool_t *tmp_pool = svn_pool_create(cfg->pool);
             const char *x_default;
-            expand_option_value(cfg, sec, default_value, &x_default, tmp_pool);
-            if (x_default)
+            if (!expand_option_value(cfg, sec, default_value, &x_default,
+                                     tmp_pool))
+              {
+                /* Recursive definitions are not supported.
+                   Normalize the answer in that case. */
+                *valuep = "";
+              }
+            else if (x_default)
               {
                 svn_stringbuf_set(cfg->tmp_value, x_default);
                 *valuep = cfg->tmp_value->data;
@@ -758,7 +823,7 @@ svn_config_set(svn_config_t *cfg,
     {
       /* Replace the option's value. */
       opt->value = apr_pstrdup(cfg->pool, value);
-      opt->expanded = FALSE;
+      opt->state = option_state_needs_expanding;
       return;
     }
 
@@ -1171,7 +1236,7 @@ svn_config_dup(svn_config_t **cfgp,
 
       destopt->value = apr_pstrdup(pool, srcopt->value);
       destopt->x_value = apr_pstrdup(pool, srcopt->x_value);
-      destopt->expanded = srcopt->expanded;
+      destopt->state = srcopt->state;
       apr_hash_set(destsec->options,
                    apr_pstrdup(pool, (const char*)optkey),
                    optkeyLength, destopt);

Modified: subversion/trunk/subversion/tests/libsvn_subr/config-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/config-test.c?rev=1688028&r1=1688027&r2=1688028&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/config-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/config-test.c Sun Jun 28 17:50:20 2015
@@ -367,6 +367,18 @@ test_expand(const svn_test_opts_t *opts,
      of "c" was not created in a temporary pool when expanding "g". */
   SVN_TEST_STRING_ASSERT(val, "bar");
 
+  /* Get expanded "j" and "k" which have cyclic definitions.
+   * They must return empty values. */
+  svn_config_get(cfg, &val, "section1", "j", NULL);
+  SVN_TEST_STRING_ASSERT(val, "");
+  svn_config_get(cfg, &val, "section1", "k", NULL);
+  SVN_TEST_STRING_ASSERT(val, "");
+
+  /* Get expanded "l" which depends on a cyclic definition.
+   * So, it also considered "undefined" and will be normalized to "". */
+  svn_config_get(cfg, &val, "section1", "l", NULL);
+  SVN_TEST_STRING_ASSERT(val, "");
+
   return SVN_NO_ERROR;
 }
 

Modified: subversion/trunk/subversion/tests/libsvn_subr/config-test.cfg
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/config-test.cfg?rev=1688028&r1=1688027&r2=1688028&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/config-test.cfg (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/config-test.cfg Sun Jun 28 17:50:20 2015
@@ -40,6 +40,11 @@ g=lyrical %(c)sd
 h=  %(unterminated
 # Multiple expansions
 i=%(a)s %(b)s
+# Recursive two-level variable expansion with surrounding text
+j=some %(k)scle
+k=c%(j)sy
+# Depends on a cyclic definition
+l=depends on a %(j)scycle!
 
 [UpperCaseSection]
 a=Aa



Mime
View raw message