Subject: Re: bug#20104: [PATCH] gzip: make the GZIP env var obsolescent

From: Paul Eggert <[email-masked]>
Date: Mon, 16 Mar 2015 14:41:18 -0700

On 03/15/2015 08:40 PM, Jim Meyering wrote:
> Sounds good.
> I too prefer to deprecate, and then (a release or two later) to remove entirely.
OK, thanks, here's a draft patch to do that.
>From 9876b9c6694ffaf12cad355484fa1f5fe482f0ce Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Mon, 16 Mar 2015 14:25:17 -0700
Subject: [PATCH] gzip: make the GZIP env var obsolescent

* NEWS, gzip.1:
* doc/gzip.texi (Environment, Tapes): Document this.
* gzip.c (args): Remove static var; no longer needed now that
'main' frees it.  All uses removed.
(ENV_OPTION, shortopts): New constants.
(main): Warn about nontrivial uses of GZIP.  Reject dangerous uses.
* tests/gzip-env: New test case.
* tests/Makefile.am (TESTS): Add it.
* util.c (add_envopt): Create new vector instead of adding to old
one.  Only use changed.
---
 NEWS              |  6 ++++
 doc/gzip.texi     | 43 ++++++++++++--------------
 gzip.1            | 46 +++++++++++++++++-----------
 gzip.c            | 91 +++++++++++++++++++++++++++++++++++++++++++++++++------
 tests/Makefile.am |  1 +
 tests/gzip-env    | 43 ++++++++++++++++++++++++++
 util.c            | 23 +++++++-------
 7 files changed, 190 insertions(+), 63 deletions(-)
 create mode 100755 tests/gzip-env

diff --git a/NEWS b/NEWS
index 4203f56..120fb93 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,12 @@ GNU gzip NEWS                                    -*- outline -*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Changes in behavior
+
+  The GZIP environment variable is now obsolescent; gzip now warns if
+  it is used, and rejects attempts to use dangerous options or operands.
+  You can use an alias or script instead.
+
 ** Bug fixes
 
   gzip -k -v no longer reports that files are replaced.
diff --git a/doc/gzip.texi b/doc/gzip.texi
index 2f4f1ab..5754aae 100644
--- a/doc/gzip.texi
+++ b/doc/gzip.texi
@@ -469,18 +469,28 @@ complement to @command{tar}, not as a replacement.
 @chapter Environment
 @cindex Environment
 
-The environment variable @env{GZIP} can hold a set of default options for
-@command{gzip}.  These options are interpreted first and can be overwritten by
-explicit command line parameters.  For example:
+The obsolescent environment variable @env{GZIP} can hold a set of
+default options for @command{gzip}.  These options are interpreted
+first and can be overwritten by explicit command line parameters.  As
+this can cause problems when using scripts, this feature is supported
+only for options that are reasonably likely to not cause too much
+harm, and @command{gzip} warns if it is used.  This feature will be
+removed in a future release of @command{gzip}.
+
+You can use an alias or script instead.  For example, if
+@command{gzip} is in the directory @samp{/usr/bin} you can prepend
+@file{$HOME/bin} to your @env{PATH} and create an executable script
+@file{$HOME/bin/gzip} containing the following:
 
 @example
-for sh:    GZIP="-8v --name"; export GZIP
-for csh:   setenv GZIP "-8v --name"
-for MSDOS: set GZIP=-8v --name
+#! /bin/sh
+export PATH=/usr/bin
+exec gzip -9 "$@@"
 @end example
 
-On @abbr{VMS}, the name of the environment variable is @env{GZIP_OPT}, to
-avoid a conflict with the symbol set for invocation of the program.
+On @abbr{VMS}, the name of the obsolescent environment variable is
+@env{GZIP_OPT}, to avoid a conflict with the symbol set for invocation
+of the program.
 
 @node Tapes
 @chapter Using @command{gzip} on tapes
@@ -491,21 +501,8 @@ the output with zeroes up to a block boundary.  When the data is read and
 the whole block is passed to @command{gunzip} for decompression,
 @command{gunzip} detects that there is extra trailing garbage after the
 compressed data and emits a warning by default if the garbage contains
-nonzero bytes.  You have to use the
-@option{--quiet} option to suppress the warning.  This option can be set in the
-@env{GZIP} environment variable, as in:
-
-@example
-for sh:    GZIP="-q"  tar -xfz --block-compress /dev/rst0
-for csh:   (setenv GZIP "-q"; tar -xfz --block-compress /dev/rst0)
-@end example
-
-In the above example, @command{gzip} is invoked implicitly by the @option{-z}
-option of @acronym{GNU} @command{tar}.  Make sure that the same block
-size (@option{-b}
-option of @command{tar}) is used for reading and writing compressed data on
-tapes.  (This example assumes you are using the @acronym{GNU} version of
-@command{tar}.)
+nonzero bytes.  You can use the @option{--quiet} option to suppress
+the warning.
 
 @node Problems
 @chapter Reporting Bugs
diff --git a/gzip.1 b/gzip.1
index d81c45a..23987e2 100644
--- a/gzip.1
+++ b/gzip.1
@@ -364,17 +364,36 @@ such as tar or zip. GNU tar supports the -z option to invoke gzip
 transparently. gzip is designed as a complement to tar, not as a
 replacement.
 .SH "ENVIRONMENT"
-The environment variable
+The obsolescent environment variable
 .B GZIP
 can hold a set of default options for
 .IR gzip .
-These options are interpreted first and can be overwritten by
-explicit command line parameters. For example:
-      for sh:    GZIP="-8v --name"; export GZIP
-      for csh:   setenv GZIP "-8v --name"
-      for MSDOS: set GZIP=-8v --name
+These options are interpreted first and can be overwritten by explicit
+command line parameters.  As this can cause problems when using
+scripts, this feature is supported only for options that are
+reasonably likely to not cause too much harm, and
+.I gzip
+warns if it is used.
+This feature will be removed in a future release of
+.IR gzip .
+.PP
+You can use an alias or script instead.  For example, if
+.I gzip
+is in the directory
+.B /usr/bin
+you can prepend
+.B $HOME/bin
+to your
+.B PATH
+and create an executable script
+.B $HOME/bin/gzip
+containing the following:
 
-On Vax/VMS, the name of the environment variable is GZIP_OPT, to
+      #! /bin/sh
+      export PATH=/usr/bin
+      exec gzip \-9 "$@"
+
+On VMS, the name of the obsolescent environment variable is GZIP_OPT, to
 avoid a conflict with the symbol set for invocation of the program.
 .SH "SEE ALSO"
 znew(1), zcmp(1), zmore(1), zforce(1), gzexe(1), zip(1), unzip(1), compress(1)
@@ -454,17 +473,8 @@ read and the whole block is passed to
 for decompression,
 .I gunzip
 detects that there is extra trailing garbage after the compressed data
-and emits a warning by default. You have to use the --quiet option to
-suppress the warning. This option can be set in the
-.B GZIP
-environment variable as in:
-  for sh:  GZIP="-q"  tar -xfz --block-compress /dev/rst0
-  for csh: (setenv GZIP -q; tar -xfz --block-compr /dev/rst0
-
-In the above example, gzip is invoked implicitly by the -z option of
-GNU tar. Make sure that the same block size (-b option of tar) is used
-for reading and writing compressed data on tapes.  (This example
-assumes you are using the GNU version of tar.)
+and emits a warning by default.  You can use the --quiet option to
+suppress the warning.
 .SH BUGS
 The gzip format represents the input size modulo 2^32, so the
 --list option reports incorrect uncompressed sizes and compression
diff --git a/gzip.c b/gzip.c
index e69c3e0..19ea909 100644
--- a/gzip.c
+++ b/gzip.c
@@ -188,7 +188,6 @@ static int part_nb;          /* number of parts in .gz file */
        struct timespec time_stamp; /* original time stamp (modification time) */
        off_t ifile_size;      /* input file size, -1 for devices (debug only) */
 static char *env;            /* contents of GZIP env variable */
-static char **args = NULL;   /* argv pointer if GZIP env variable defined */
 static char const *z_suffix; /* default suffix (can be set with --suffix) */
 static size_t z_len;         /* strlen(z_suffix) */
 
@@ -242,9 +241,15 @@ static int handled_sig[] =
    non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
 enum
 {
-  PRESUME_INPUT_TTY_OPTION = CHAR_MAX + 1
+  PRESUME_INPUT_TTY_OPTION = CHAR_MAX + 1,
+
+  /* A value greater than all valid long options, used as a flag to
+     distinguish options derived from the GZIP environment variable.  */
+  ENV_OPTION
 };
 
+static char const shortopts[] = "ab:cdfhH?klLmMnNqrS:tvVZ123456789";
+
 static const struct option longopts[] =
 {
  /* { name  has_arg  *flag  val } */
@@ -401,7 +406,9 @@ int main (int argc, char **argv)
 {
     int file_count;     /* number of files to process */
     size_t proglen;     /* length of program_name */
-    int optc;           /* current option */
+    char **argv_copy;
+    int env_argc;
+    char **env_argv;
 
     EXPAND(argc, argv); /* wild card expansion if necessary */
 
@@ -415,8 +422,9 @@ int main (int argc, char **argv)
       program_name[proglen - 4] = '\0';
 
     /* Add options in GZIP environment variable if there is one */
-    env = add_envopt(&argc, &argv, OPTIONS_VAR);
-    if (env != NULL) args = argv;
+    argv_copy = argv;
+    env = add_envopt (&env_argc, &argv_copy, OPTIONS_VAR);
+    env_argv = env ? argv_copy : NULL;
 
 #ifndef GNU_STANDARD
 # define GNU_STANDARD 1
@@ -440,8 +448,53 @@ int main (int argc, char **argv)
     z_suffix = Z_SUFFIX;
     z_len = strlen(z_suffix);
 
-    while ((optc = getopt_long (argc, argv, "ab:cdfhH?klLmMnNqrS:tvVZ123456789",
-                                longopts, (int *)0)) != -1) {
+    while (true) {
+        int optc;
+        int longind = -1;
+
+        if (env_argv)
+          {
+            if (env_argv[optind] && strequ (env_argv[optind], "--"))
+              optc = ENV_OPTION + '-';
+            else
+              {
+                optc = getopt_long (env_argc, env_argv, shortopts, longopts,
+                                    &longind);
+                if (0 <= optc)
+                  optc += ENV_OPTION;
+                else
+                  {
+                    if (optind != env_argc)
+                      {
+                        fprintf (stderr,
+                                 ("%s: %s: non-option in "OPTIONS_VAR
+                                  " environment variable\n"),
+                                 program_name, env_argv[optind]);
+                        try_help ();
+                      }
+
+                    /* Wait until here before warning, so that GZIP='-q'
+                       doesn't warn.  */
+                    if (env_argc != 1 && !quiet)
+                      fprintf (stderr,
+                               ("%s: warning: "OPTIONS_VAR" environment variable"
+                                " is deprecated; use an alias or script\n"),
+                               program_name);
+
+                    /* Start processing ARGC and ARGV instead.  */
+                    free (env_argv);
+                    env_argv = NULL;
+                    optind = 1;
+                    longind = -1;
+                  }
+              }
+          }
+
+        if (!env_argv)
+          optc = getopt_long (argc, argv, shortopts, longopts, &longind);
+        if (optc < 0)
+          break;
+
         switch (optc) {
         case 'a':
             ascii = 1; break;
@@ -474,12 +527,15 @@ int main (int argc, char **argv)
         case 'M': /* undocumented, may change later */
             no_time = 0; break;
         case 'n':
+        case 'n' + ENV_OPTION:
             no_name = no_time = 1; break;
         case 'N':
+        case 'N' + ENV_OPTION:
             no_name = no_time = 0; break;
         case PRESUME_INPUT_TTY_OPTION:
             presume_input_tty = true; break;
         case 'q':
+        case 'q' + ENV_OPTION:
             quiet = 1; verbose = 0; break;
         case 'r':
 #if NO_DIR
@@ -501,6 +557,7 @@ int main (int argc, char **argv)
             test = decompress = to_stdout = 1;
             break;
         case 'v':
+        case 'v' + ENV_OPTION:
             verbose++; quiet = 0; break;
         case 'V':
             version(); do_exit(OK); break;
@@ -513,12 +570,28 @@ int main (int argc, char **argv)
             try_help ();
             break;
 #endif
+        case '1' + ENV_OPTION:  case '2' + ENV_OPTION:  case '3' + ENV_OPTION:
+        case '4' + ENV_OPTION:  case '5' + ENV_OPTION:  case '6' + ENV_OPTION:
+        case '7' + ENV_OPTION:  case '8' + ENV_OPTION:  case '9' + ENV_OPTION:
+            optc -= ENV_OPTION;
+            /* Fall through.  */
         case '1':  case '2':  case '3':  case '4':
         case '5':  case '6':  case '7':  case '8':  case '9':
             level = optc - '0';
             break;
+
         default:
-            /* Error message already emitted by getopt_long. */
+            if (ENV_OPTION <= optc && optc != ENV_OPTION + '?')
+              {
+                /* Output a diagnostic, since getopt_long didn't.  */
+                fprintf (stderr, "%s: ", program_name);
+                if (0 <= longind)
+                  fprintf (stderr, "--%s: ", longopts[longind].name);
+                else
+                  fprintf (stderr, "-%c: ", optc - ENV_OPTION);
+                fprintf (stderr, ("option not valid in "OPTIONS_VAR
+                                  " environment variable\n"));
+              }
             try_help ();
         }
     } /* loop on all arguments */
@@ -1902,8 +1975,6 @@ local void do_exit(exitcode)
     in_exit = 1;
     free(env);
     env  = NULL;
-    free(args);
-    args = NULL;
     FREE(inbuf);
     FREE(outbuf);
     FREE(d_buf);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8dd6436..8252670 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 TESTS =					\
+  gzip-env				\
   helin-segv				\
   help-version				\
   hufts					\
diff --git a/tests/gzip-env b/tests/gzip-env
new file mode 100755
index 0000000..90ceab3
--- /dev/null
+++ b/tests/gzip-env
@@ -0,0 +1,43 @@
+#!/bin/sh
+# Test the obsolescent GZIP environment variable.
+
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+# limit so don't run it by default.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ..
+
+echo a >exp || framework_failure_
+gzip <exp >in || framework_failure_
+
+fail=0
+GZIP=-qv gzip -d <in >out 2>err || fail=1
+compare exp out || fail=1
+
+for badopt in -- -c --stdout -d --decompress -f --force -h --help -k --keep \
+  -l --list -L --license -r --recursive -Sxxx --suffix=xxx '--suffix xxx' \
+  -t --test -V --version
+do
+  GZIP=$badopt gzip -d <in >out 2>err && fail=1
+done
+
+for goodopt in -n --no-name -N --name -q --quiet -v --verbose \
+  -1 --fast -2 -3 -4 -5 -6 -7 -8 -9 --best
+do
+  GZIP=$goodopt gzip -d <in >out 2>err || fail=1
+  compare exp out || fail=1
+done
+
+Exit $fail
diff --git a/util.c b/util.c
index 881c6ee..f84dfc9 100644
--- a/util.c
+++ b/util.c
@@ -361,11 +361,15 @@ void make_simple_name(name)
     } while (p != name);
 }
 
-/* ========================================================================
- * Add an environment variable (if any) before argv, and update argc.
- * Return the expanded environment variable to be freed later, or NULL
- * if no options were added to argv.
- */
+/* Convert the value of the environment variable ENVVAR_NAME
+   to a newly allocated argument vector, and set *ARGCP and *ARGVP
+   to the number of arguments and to the vector, respectively.
+   Make the new vector's zeroth element equal to the old **ARGVP.
+   Return a pointer to the newly allocated string storage.
+
+   If the vector would be empty, do not allocate storage,
+   do not set *ARGCP and *ARGVP, and return NULL.  */
+
 #define SEPARATOR	" \t"	/* separators in env variable */
 
 char *add_envopt(
@@ -376,7 +380,6 @@ char *add_envopt(
     char *p;             /* running pointer through env variable */
     char **oargv;        /* runs through old argv array */
     char **nargv;        /* runs through new argv array */
-    int	 oargc = *argcp; /* old argc */
     int  nargc = 0;      /* number of arguments in env variable */
     char *env_val;
 
@@ -396,7 +399,7 @@ char *add_envopt(
         free(env_val);
         return NULL;
     }
-    *argcp += nargc;
+    *argcp = nargc + 1;
     /* Allocate the new argv array, with an extra element just in case
      * the original arg list did not end with a NULL.
      */
@@ -405,9 +408,7 @@ char *add_envopt(
     *argvp = nargv;
 
     /* Copy the program name first */
-    if (oargc-- < 0)
-      gzip_error ("argc<=0");
-    *(nargv++) = *(oargv++);
+    *nargv++ = *oargv;
 
     /* Then copy the environment args */
     for (p = env_val; nargc > 0; nargc--) {
@@ -416,8 +417,6 @@ char *add_envopt(
         while (*p++) ;			     /* skip over word */
     }
 
-    /* Finally copy the old args and add a NULL (usual convention) */
-    while (oargc--) *(nargv++) = *(oargv++);
     *nargv = NULL;
     return env_val;
 }
-- 
2.1.0