Subject: Re: bug#47412: env: fragile argument parsing

From: Paul Eggert <[email-masked]>
Date: Mon, 29 Mar 2021 22:06:15 -0700

On 3/26/21 3:21 PM, Paul Eggert wrote:
> The -S code could use some more fixes in this area too - it can 
> probably still dump core on platforms like the Hurd that don't limit 
> exec arg size - but one thing at a time. 

I fixed the (unlikely) bugs I found in this area by installing the attached.

>From e3766c5db176ca7abbb8212d5b0b7862fb98a5be Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Mon, 29 Mar 2021 21:42:44 -0700
Subject: [PATCH] env: simplify --split-string memory management

* bootstrap.conf (gnulib_modules): Add idx.
* src/env.c: Include idx.h, minmax.h.
Prefer idx_t to ptrdiff_t when values are nonnegative.
(valid_escape_sequence, escape_char, validate_split_str)
(CHECK_START_NEW_ARG):
Remove; no longer needed now that we validate as we go.
(struct splitbuf): New type.
(splitbuf_grow, splitbuf_append_byte, check_start_new_arg)
(splitbuf_finishup): New functions.
(build_argv): New arg ARGC.  Validate and process in one go, using
the new functions; this is simpler and more reliable than the old
approach (as witness the recent bug).  Avoid integer overflow in
the unlikely case where the string contains more than INT_MAX
arguments.
(parse_split_string): Simplify by exploiting the new build_argv.
---
 bootstrap.conf |   1 +
 src/env.c      | 385 ++++++++++++++++++++++---------------------------
 2 files changed, 173 insertions(+), 213 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index ab6b3ef0c..f55da99db 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -134,6 +134,7 @@ gnulib_modules="
   host-os
   human
   idcache
+  idx
   ignore-value
   inttostr
   inttypes
diff --git a/src/env.c b/src/env.c
index 11db374d9..e2ab39fd5 100644
--- a/src/env.c
+++ b/src/env.c
@@ -26,6 +26,8 @@
 #include "system.h"
 #include "die.h"
 #include "error.h"
+#include "idx.h"
+#include "minmax.h"
 #include "operand2sig.h"
 #include "quote.h"
 #include "sig2str.h"
@@ -41,14 +43,14 @@
 /* Array of envvars to unset.  */
 static const char **usvars;
 static size_t usvars_alloc;
-static ptrdiff_t usvars_used;
+static idx_t usvars_used;
 
 /* Annotate the output with extra info to aid the user.  */
 static bool dev_debug;
 
 /* Buffer and length of extracted envvars in -S strings.  */
 static char *varname;
-static ptrdiff_t vnlen;
+static idx_t vnlen;
 
 /* Possible actions on each signal.  */
 enum SIGNAL_MODE {
@@ -175,7 +177,7 @@ append_unset_var (const char *var)
 static void
 unset_envvars (void)
 {
-  for (ptrdiff_t i = 0; i < usvars_used; ++i)
+  for (idx_t i = 0; i < usvars_used; ++i)
     {
       devmsg ("unset:    %s\n", usvars[i]);
 
@@ -190,29 +192,6 @@ unset_envvars (void)
   IF_LINT (usvars_alloc = 0);
 }
 
-static bool _GL_ATTRIBUTE_PURE
-valid_escape_sequence (const char c)
-{
-  return (c == 'c' || c == 'f' || c == 'n' || c == 'r' || c == 't' || c == 'v' \
-          || c == '#' || c == '$' || c == '_' || c == '"' || c == '\'' \
-          || c == '\\');
-}
-
-static char _GL_ATTRIBUTE_PURE
-escape_char (const char c)
-{
-  switch (c)
-    {
-    /* \a, \b not supported by FreeBSD's env.  */
-    case 'f': return '\f';
-    case 'n': return '\n';
-    case 'r': return '\r';
-    case 't': return '\t';
-    case 'v': return '\v';
-    default: assume (false);
-    }
-}
-
 /* Return a pointer to the end of a valid ${VARNAME} string, or NULL.
    'str' should point to the '$' character.
    First letter in VARNAME must be alpha or underscore,
@@ -241,7 +220,7 @@ scan_varname (const char *str)
 static char *
 extract_varname (const char *str)
 {
-  ptrdiff_t i;
+  idx_t i;
   const char *p;
 
   p = scan_varname (str);
@@ -263,150 +242,127 @@ extract_varname (const char *str)
   return varname;
 }
 
-/* Validate the "-S" parameter, according to the syntax defined by FreeBSD's
-   env(1).  Terminate with an error message if not valid.
+/* Temporary buffer used by --split-string processing.  */
+struct splitbuf
+{
+  /* Buffer address, arg count, and half the number of elements in the buffer.
+     ARGC and ARGV are as in 'main', and ARGC + 1 <= HALF_ALLOC so
+     that the upper half of ARGV can be used for string contents.
+     This may waste up to half the space but keeps the code simple,
+     which is better for this rarely-used but security-sensitive code.
+
+     ARGV[0] is not initialized; that is the caller's responsibility
+     after finalization.
+
+     During assembly, ARGV[I] (where 0 < I < ARGC) contains the offset
+     of the Ith string (relative to ARGV + HALF_ALLOC), so that
+     reallocating ARGV does not change the validity of its contents.
+     The integer offset is cast to char * during assembly, and is
+     converted to a true char * pointer on finalization.
+
+     During assembly, ARGV[ARGC] contains the offset of the first
+     unused string byte (relative to ARGV + HALF_ALLOC).  */
+  char **argv;
+  int argc;
+  idx_t half_alloc;
+
+  /* The number of extra argv slots to keep room for.  */
+  int extra_argc;
+
+  /* Whether processing should act as if the most recent character
+     seen was a separator.  */
+  bool sep;
+};
 
-   Calculate and set two values:
-   bufsize - the size (in bytes) required to hold the resulting string
-             after ENVVAR expansion (the value is overestimated).
-   maxargc - the maximum number of arguments (the size of the new argv).  */
+/* Expand SS so that it has at least one more argv slot and at least
+   one more string byte.  */
 static void
-validate_split_str (const char *str, ptrdiff_t *bufsize, int *maxargc)
+splitbuf_grow (struct splitbuf *ss)
 {
-  bool dq, sq, sp;
-  const char *pch;
-  ptrdiff_t buflen;
-  int cnt = 1;
+  idx_t old_half_alloc = ss->half_alloc;
+  idx_t string_bytes = (intptr_t) ss->argv[ss->argc];
+  ss->argv = xpalloc (ss->argv, &ss->half_alloc, 1,
+                      MIN (INT_MAX, IDX_MAX), 2 * sizeof *ss->argv);
+  memmove (ss->argv + ss->half_alloc, ss->argv + old_half_alloc, string_bytes);
+}
 
-  dq = sq = sp = false;
-  buflen = strlen (str) + 1;
+/* In SS, append C to the last string.  */
+static void
+splitbuf_append_byte (struct splitbuf *ss, char c)
+{
+  idx_t string_bytes = (intptr_t) ss->argv[ss->argc];
+  if (ss->half_alloc * sizeof *ss->argv <= string_bytes)
+    splitbuf_grow (ss);
+  ((char *) (ss->argv + ss->half_alloc))[string_bytes] = c;
+  ss->argv[ss->argc] = (char *) (intptr_t) (string_bytes + 1);
+}
 
-  while (*str)
+/* If SS's most recent character was a separator, finish off its
+   previous argument and start a new one.  */
+static void
+check_start_new_arg (struct splitbuf *ss)
+{
+  if (ss->sep)
     {
-      const char next = str[1];
-
-      if (c_isspace (*str) && !dq && !sq)
-        {
-          sp = true;
-        }
-      else
-        {
-          if (sp)
-            ++cnt;
-          sp = false;
-        }
-
-      switch (*str)
-        {
-        case '\'':
-          sq = !sq && !dq;
-          break;
-
-        case '"':
-          dq = !sq && !dq;
-          break;
-
-        case '\\':
-          if (dq && next == 'c')
-            die (EXIT_CANCELED, 0,
-                 _("'\\c' must not appear in double-quoted -S string"));
-
-          if (next == '\0')
-            die (EXIT_CANCELED, 0,
-                 _("invalid backslash at end of string in -S"));
-
-          if (!valid_escape_sequence (next))
-            die (EXIT_CANCELED, 0, _("invalid sequence '\\%c' in -S"), next);
-
-          if (next == '_')
-            ++cnt;
-
-          ++str;
-          break;
-
-
-        case '$':
-          if (sq)
-            break;
-
-          pch = extract_varname (str);
-          if (! pch)
-            die (EXIT_CANCELED, 0, _("only ${VARNAME} expansion is supported,"\
-                                     " error at: %s"), str);
-
-          pch = getenv (pch);
-          if (pch)
-            buflen += strlen (pch);
-          break;
-        }
-      ++str;
+      splitbuf_append_byte (ss, '\0');
+      int argc = ss->argc;
+      if (ss->half_alloc <= argc + ss->extra_argc + 1)
+        splitbuf_grow (ss);
+      ss->argv[argc + 1] = ss->argv[argc];
+      ss->argc = argc + 1;
+      ss->sep = false;
     }
+}
 
-  if (dq || sq)
-    die (EXIT_CANCELED, 0, _("no terminating quote in -S string"));
-
-  *maxargc = cnt;
-  *bufsize = buflen;
+/* All additions to SS have been made.  Convert its offsets to pointers,
+   and return the resulting argument vector.  */
+static char **
+splitbuf_finishup (struct splitbuf *ss)
+{
+  int argc = ss->argc;
+  char **argv = ss->argv;
+  char *stringbase = (char *) (ss->argv + ss->half_alloc);
+  for (int i = 1; i < argc; i++)
+    argv[i] = stringbase + (intptr_t) argv[i];
+  return argv;
 }
 
-/* Return a newly-allocated *arg[]-like array,
+/* Return a newly-allocated argv-like array,
    by parsing and splitting the input 'str'.
+
    'extra_argc' is the number of additional elements to allocate
    in the array (on top of the number of args required to split 'str').
 
+   Store into *argc the number of arguments found (plus 1 for
+   the program name).
+
    Example:
-     char **argv = build_argv ("A=B uname -k', 3)
+     int argc;
+     char **argv = build_argv ("A=B uname -k', 3, &argc);
    Results in:
-     argv[0] = "DUMMY" - dummy executable name, can be replaced later.
+     argc = 4
+     argv[0] = [not initialized]
      argv[1] = "A=B"
      argv[2] = "uname"
      argv[3] = "-k"
-     argv[4] = NULL
-     argv[5,6,7] = [allocated due to extra_argc, but not initialized]
+     argv[4,5,6,7] = [allocated due to extra_argc + 1, but not initialized]
 
-   The strings are stored in an allocated buffer, pointed by argv[0].
    To free allocated memory:
-     free (argv[0]);
-     free (argv); */
+     free (argv);
+   However, 'env' does not free since it's about to exec or exit anyway
+   and the complexity of keeping track of the storage that may have been
+   allocated via multiple calls to build_argv is not worth the hassle.  */
 static char **
-build_argv (const char *str, int extra_argc)
+build_argv (const char *str, int extra_argc, int *argc)
 {
-  bool dq = false, sq = false, sep = true;
-
-  /* Buffer to hold the new argv values.  Allocated as one buffer, but
-     will contain multiple NUL-terminate strings.  */
-  char *dest;
-
-  char **newargv, **nextargv;
-  int newargc = 0;
-  ptrdiff_t buflen = 0;
-
-  /* This macro is called before inserting any characters to the output
-     buffer.  It checks if the previous character was a separator
-     and if so starts a new argv element.  */
-#define CHECK_START_NEW_ARG                     \
-  do {                                          \
-    if (sep)                                    \
-      {                                         \
-        *dest++ = '\0';                         \
-        *nextargv++ = dest;                     \
-        sep = false;                            \
-      }                                         \
-  } while (0)
-
-  validate_split_str (str, &buflen, &newargc);
-
-  /* Allocate buffer.  +6 for the "DUMMY\0" executable name, +1 for NUL.  */
-  dest = xmalloc (buflen + 6 + 1);
-
-  /* Allocate the argv array.
-     +2 for the program name (argv[0]) and the last NULL pointer.  */
-  nextargv = newargv = xmalloc ((newargc + extra_argc + 2) * sizeof (char *));
-
-  /* argv[0] = executable's name  - will be replaced later.  */
-  strcpy (dest, "DUMMY");
-  *nextargv++ = dest;
-  dest += 6;
+  bool dq = false, sq = false;
+  struct splitbuf ss;
+  ss.argv = xnmalloc (extra_argc + 2, 2 * sizeof *ss.argv);
+  ss.argc = 1;
+  ss.half_alloc = extra_argc + 2;
+  ss.extra_argc = extra_argc;
+  ss.sep = true;
+  ss.argv[ss.argc] = 0;
 
   /* In the following loop,
      'break' causes the character 'newc' to be added to *dest,
@@ -421,7 +377,7 @@ build_argv (const char *str, int extra_argc)
           if (dq)
             break;
           sq = !sq;
-          CHECK_START_NEW_ARG;
+          check_start_new_arg (&ss);
           ++str;
           continue;
 
@@ -429,7 +385,7 @@ build_argv (const char *str, int extra_argc)
           if (sq)
             break;
           dq = !dq;
-          CHECK_START_NEW_ARG;
+          check_start_new_arg (&ss);
           ++str;
           continue;
 
@@ -437,12 +393,12 @@ build_argv (const char *str, int extra_argc)
           /* Start a new argument if outside quotes.  */
           if (sq || dq)
             break;
-          sep = true;
+          ss.sep = true;
           str += strspn (str, C_ISSPACE_CHARS);
           continue;
 
         case '#':
-          if (!sep)
+          if (!ss.sep)
             break;
           goto eos; /* '#' as first char terminates the string.  */
 
@@ -454,26 +410,41 @@ build_argv (const char *str, int extra_argc)
 
           /* Skip the backslash and examine the next character.  */
           newc = *++str;
-          if (newc == '\\' || newc == '\''
-              || (!sq && (newc == '#' || newc == '$' || newc == '"')))
+          switch (newc)
             {
+            case '"': case '#': case '$': case '\'': case '\\':
               /* Pass escaped character as-is.  */
-            }
-          else if (newc == '_')
-            {
+              break;
+
+            case '_':
               if (!dq)
                 {
                   ++str;  /* '\_' outside double-quotes is arg separator.  */
-                  sep = true;
+                  ss.sep = true;
                   continue;
                 }
-              else
-                newc = ' ';  /* '\_' inside double-quotes is space.  */
+              newc = ' ';  /* '\_' inside double-quotes is space.  */
+              break;
+
+            case 'c':
+              if (dq)
+                die (EXIT_CANCELED, 0,
+                     _("'\\c' must not appear in double-quoted -S string"));
+              goto eos; /* '\c' terminates the string.  */
+
+            case 'f': newc = '\f'; break;
+            case 'n': newc = '\n'; break;
+            case 'r': newc = '\r'; break;
+            case 't': newc = '\t'; break;
+            case 'v': newc = '\v'; break;
+
+            case '\0':
+              die (EXIT_CANCELED, 0,
+                   _("invalid backslash at end of string in -S"));
+
+            default:
+              die (EXIT_CANCELED, 0, _("invalid sequence '\\%c' in -S"), newc);
             }
-          else if (newc == 'c')
-            goto eos; /* '\c' terminates the string.  */
-          else
-            newc = escape_char (newc);  /* Other characters (e.g., '\n').  */
           break;
 
         case '$':
@@ -484,12 +455,18 @@ build_argv (const char *str, int extra_argc)
           /* Store the ${VARNAME} value.  */
           {
             char *n = extract_varname (str);
+            if (!n)
+              die (EXIT_CANCELED, 0,
+                   _("only ${VARNAME} expansion is supported, error at: %s"),
+                   str);
+
             char *v = getenv (n);
             if (v)
               {
-                CHECK_START_NEW_ARG;
+                check_start_new_arg (&ss);
                 devmsg ("expanding ${%s} into %s\n", n, quote (v));
-                dest = stpcpy (dest, v);
+                for (; *v; v++)
+                  splitbuf_append_byte (&ss, *v);
               }
             else
               devmsg ("replacing ${%s} with null string\n", n);
@@ -499,16 +476,18 @@ build_argv (const char *str, int extra_argc)
           }
         }
 
-      CHECK_START_NEW_ARG;
-      *dest++ = newc;
+      check_start_new_arg (&ss);
+      splitbuf_append_byte (&ss, newc);
       ++str;
     }
 
- eos:
-  *dest = '\0';
-  *nextargv = NULL; /* Mark the last element in argv as NULL.  */
+  if (dq || sq)
+    die (EXIT_CANCELED, 0, _("no terminating quote in -S string"));
 
-  return newargv;
+ eos:
+  splitbuf_append_byte (&ss, '\0');
+  *argc = ss.argc;
+  return splitbuf_finishup (&ss);
 }
 
 /* Process an "-S" string and create the corresponding argv array.
@@ -517,67 +496,47 @@ build_argv (const char *str, int extra_argc)
    Example: if executed as:
       $ env -S"-i -C/tmp A=B" foo bar
    The input argv is:
-      argv[0] = 'env'
+      argv[0] = "env"
       argv[1] = "-S-i -C/tmp A=B"
-      argv[2] = foo
-      argv[3] = bar
+      argv[2] = "foo"
+      argv[3] = "bar"
+      argv[4] = NULL
    This function will modify argv to be:
-      argv[0] = 'env'
+      argv[0] = "env"
       argv[1] = "-i"
       argv[2] = "-C/tmp"
-      argv[3] =  A=B"
-      argv[4] = foo
-      argv[5] = bar
+      argv[3] = "A=B"
+      argv[4] = "foo"
+      argv[5] = "bar"
+      argv[6] = NULL
    argc will be updated from 4 to 6.
    optind will be reset to 0 to force getopt_long to rescan all arguments.  */
 static void
 parse_split_string (const char *str, int *orig_optind,
                     int *orig_argc, char ***orig_argv)
 {
-  int i, newargc;
-  char **newargv, **nextargv;
-
-
-  while (c_isspace (*str))
-    str++;
-  if (*str == '\0')
-    return;
-
-  newargv = build_argv (str, *orig_argc - *orig_optind);
+  int extra_argc = *orig_argc - *orig_optind, newargc;
+  char **newargv = build_argv (str, extra_argc, &newargc);
 
   /* Restore argv[0] - the 'env' executable name.  */
   *newargv = (*orig_argv)[0];
 
-  /* Start from argv[1] */
-  nextargv = newargv + 1;
-
-  /* Print parsed arguments */
-  if (dev_debug && *nextargv)
+  /* Print parsed arguments.  */
+  if (dev_debug && 1 < newargc)
     {
       devmsg ("split -S:  %s\n", quote (str));
-      devmsg (" into:    %s\n", quote (*nextargv++));
-      while (*nextargv)
-        devmsg ("     &    %s\n", quote (*nextargv++));
+      devmsg (" into:    %s\n", quote (newargv[1]));
+      for (int i = 2; i < newargc; i++)
+        devmsg ("     &    %s\n", quote (newargv[i]));
     }
-  else
-    {
-      /* Ensure nextargv points to the last argument */
-      while (*nextargv)
-        ++nextargv;
-    }
-
-  /* Add remaining arguments from original command line */
-  for (i = *orig_optind; i < *orig_argc; ++i)
-    *nextargv++ = (*orig_argv)[i];
-  *nextargv = NULL;
 
-  /* Count how many new arguments we have */
-  newargc = 0;
-  for (nextargv = newargv; *nextargv; ++nextargv)
-    ++newargc;
+  /* Add remaining arguments and terminating null from the original
+     command line.  */
+  memcpy (newargv + newargc, *orig_argv + *orig_optind,
+          (extra_argc + 1) * sizeof *newargv);
 
   /* Set new values for original getopt variables.  */
-  *orig_argc = newargc;
+  *orig_argc = newargc + extra_argc;
   *orig_argv = newargv;
   *orig_optind = 0; /* Tell getopt to restart from first argument.  */
 }
-- 
2.25.1