Re: [PATCH] Use strdup in dd to avoid changing argv elements

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> Nice.  Applied.
> Thanks!

You're welcome, but it turns out that I missed a case: commas in
arguments were also zapped, e.g., "dd conv=ascii,block".  Here's a
further patch to avoid doing this, adding some "const"s to help
prevent this from recurring.

This patch changes the spelling of an error diagnostic slightly, if
the host does not have a working O_NOFOLLOW.  Instead of this:

  dd: invalid output flag: `nofollow'

The output will look like this:

  dd: invalid output flag: `nofollow'
  Try `dd --help' for more information.

I mildly prefer the new behavior but I don't think this behavior
change is important.

For what it's worth, the combined effect of this change and my
previous one is to shrink the size of the dd executable by 0.5% on my
Debian stable x86 box.

2008-01-30  Paul Eggert  <address@hidden>

        Don't modify argv in dd due to ',' in arguments.
        * src/dd.c: Include quotearg.h.
        (operand_matches): New function.
        (parse_symbols, operand_is): Use it.
        (parse_symbols): 1st arg is now const pointer.  Don't modify it.
        msgid arg is now just the message, not a format.
        (scanargs): Add some 'const's to check for problems like the above.

diff --git a/src/dd.c b/src/dd.c
index 72d9272..4a8419d 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -31,6 +31,7 @@
 #include "human.h"
 #include "long-options.h"
 #include "quote.h"
+#include "quotearg.h"
 #include "xstrtol.h"
 #include "xtime.h"
 
@@ -796,41 +797,49 @@ write_output (void)
   oc = 0;
 }
 
+/* Return true if STR is of the form "PATTERN" or "PATTERNDELIM...".  */
+
+static bool
+operand_matches (char const *str, char const *pattern, char delim)
+{
+  while (*pattern)
+    if (*str++ != *pattern++)
+      return false;
+  return !*str || *str == delim;
+}
+
 /* Interpret one "conv=..." or similar operand STR according to the
    symbols in TABLE, returning the flags specified.  If the operand
    cannot be parsed, use ERROR_MSGID to generate a diagnostic.
    As a by product, this function replaces each `,' in STR with a NUL byte.  */
 
 static int
-parse_symbols (char *str, struct symbol_value const *table,
+parse_symbols (char const *str, struct symbol_value const *table,
               char const *error_msgid)
 {
   int value = 0;
 
-  do
+  for (;;)
     {
+      char const *strcomma = strchr (str, ',');
       struct symbol_value const *entry;
-      char *new = strchr (str, ',');
-      if (new != NULL)
-       *new++ = '\0';
-      for (entry = table; ; entry++)
-       {
-         if (! entry->symbol[0])
-           {
-             error (0, 0, _(error_msgid), quote (str));
-             usage (EXIT_FAILURE);
-           }
-         if (STREQ (entry->symbol, str))
-           {
-             if (! entry->value)
-               error (EXIT_FAILURE, 0, _(error_msgid), quote (str));
-             value |= entry->value;
-             break;
-           }
-       }
-      str = new;
+
+      for (entry = table;
+          ! (operand_matches (str, entry->symbol, ',') && entry->value);
+          entry++)
+       if (! entry->symbol[0])
+         {
+           size_t slen = strcomma ? strcomma - str : strlen (str);
+           error (0, 0, "%s: %s", _(error_msgid),
+                  quotearg_n_style_mem (0, locale_quoting_style, str, slen));
+           usage (EXIT_FAILURE);
+         }
+
+      value |= entry->value;
+      if (!strcomma)
+       break;
+      str = strcomma + 1;
     }
-  while (str);
 
   return value;
 }
@@ -867,29 +876,25 @@ parse_integer (const char *str, bool *invalid)
   return n;
 }
 
-/* Return true if OPERAND is of the form "NAME=...".  */
+/* OPERAND is of the form "X=...".  Return true if X is NAME.  */
 
 static bool
 operand_is (char const *operand, char const *name)
 {
-  while (*name)
-    if (*name++ != *operand++)
-      return false;
-  return *operand == '=';
+  return operand_matches (operand, name, '=');
 }
 
 static void
-scanargs (int argc, char **argv)
+scanargs (int argc, char *const *argv)
 {
   int i;
   size_t blocksize = 0;
 
   for (i = optind; i < argc; i++)
     {
-      char *name, *val;
+      char const *name = argv[i];
+      char const *val = strchr (name, '=');
 
-      name = argv[i];
-      val = strchr (name, '=');
       if (val == NULL)
        {
          error (0, 0, _("unrecognized operand %s"), quote (name));
@@ -903,16 +908,16 @@ scanargs (int argc, char **argv)
        output_file = val;
       else if (operand_is (name, "conv"))
        conversions_mask |= parse_symbols (val, conversions,
-                                          N_("invalid conversion: %s"));
+                                          N_("invalid conversion"));
       else if (operand_is (name, "iflag"))
        input_flags |= parse_symbols (val, flags,
-                                     N_("invalid input flag: %s"));
+                                     N_("invalid input flag"));
       else if (operand_is (name, "oflag"))
        output_flags |= parse_symbols (val, flags,
-                                      N_("invalid output flag: %s"));
+                                      N_("invalid output flag"));
       else if (operand_is (name, "status"))
        status_flags |= parse_symbols (val, statuses,
-                                      N_("invalid status flag: %s"));
+                                      N_("invalid status flag"));
       else
        {
          bool invalid = false;