Subject: Re: bug#76197: [PROPOSED] gzip: drop support for the GZIP env var

From: Paul Eggert <[email-masked]>
Date: Wed, 19 Feb 2025 22:50:14 -0800

On 2025-02-12 05:58, Lasse Collin wrote:

> the removal makes gzip one
> of the few compression tools that don't support setting even the
> compression level via an environment variable.

Thanks for the review. On further thought I agree that, for backward 
compatibility reasons at least, the gzip command should continue to 
examine the GZIP environment variable for options like -9 that affect 
only performance (including compression performance). These options 
shouldn't cause the bigger glitches (and perhaps even security hazards) 
that behavior-oriented options can cause. This would be more in line 
with zstd, which takes this relatively-cautious approach to environment 
variables.

You mentioned --synchronous as being innocuous even though gzip 
currently doesn't allow it in GZIP, and that also is a good suggestion.

So I installed the attached patch instead into Savannah master gzip. It 
means gzip will pay attention to -1..-9, --rsyncable, and --synchronous 
but will silently ignore everything else in GZIP. The idea is that a 
garbage GZIP value shouldn't disrupt normal operations.

From 02cde43286da3b47182daac61352fa26ed35e2ea Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 19 Feb 2025 22:33:34 -0800
Subject: [PATCH] Ignore GZIP envvar except for innocuous flags

This is slimmed down from the originally proposed patch, which
ignored GZIP entirely.  Thanks to Lasse Collin for thoughtful comments.
* gzip.c (ENV_OPTION): Remove.  All uses removed.
(main): Parse GZIP contents with error diagnostics disabled,
and simply ignore invalid contents.  Ignore all options
except -1 thru -9, --rsyncable, --synchronous.  Remove
unnecessary longind local; all uses removed.
* tests/gzip-env (GZIP): Adjust to match current behavior.
Also test --rsyncable, --synchronous.
---
 NEWS           |  9 +++++++
 doc/gzip.texi  | 13 +++++-----
 gzip.1         | 17 ++++++-------
 gzip.c         | 69 ++++++++++++--------------------------------------
 tests/gzip-env | 16 +++++++-----
 znew.in        |  3 ++-
 6 files changed, 52 insertions(+), 75 deletions(-)

diff --git a/NEWS b/NEWS
index e348355..9341ef8 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,15 @@ GNU gzip NEWS                                    -*- outline -*-
   'gzip -S' now rejects suffixes containing '/'.
   [bug present since the beginning]
 
+** Changes in behavior
+
+  The GZIP environment variable is now silently ignored except for the
+  options -1 (--fast) through -9 (--best), --rsyncable, and --synchronous.
+  This brings gzip into line with more-cautious compressors like zstd
+  that limit environment variables' effect to relatively innocuous
+  performance issues.  You can continue to use scripts to specify
+  whatever gzip options you like.
+
 ** Performance improvements
 
   gzip now decompresses significantly faster by computing CRCs via a
diff --git a/doc/gzip.texi b/doc/gzip.texi
index cc05b60..ac03950 100644
--- a/doc/gzip.texi
+++ b/doc/gzip.texi
@@ -487,15 +487,16 @@ complement to @command{tar}, not as a replacement.
 @chapter Environment
 @cindex Environment
 
-The obsolescent environment variable @env{GZIP} can hold a set of
+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.  As
+first and can be overridden 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}.
+only for @option{--rsyncable}, @option{--synchronous}, and
+options like @option{-9} that set the compression level;
+any other options or operands in @env{GZIP} are silently ignored.
 
-You can use an alias or script instead.  For example, if
+You can use an alias or script instead.  For example,
+instead of setting @samp{GZIP="-9"} in the environment, 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:
diff --git a/gzip.1 b/gzip.1
index 0efe399..8ab131b 100644
--- a/gzip.1
+++ b/gzip.1
@@ -416,22 +416,21 @@ 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 obsolescent environment variable
+The environment variable
 .B GZIP
 can hold a set of default options for
 .BR gzip .
-These options are interpreted first and can be overwritten by explicit
+These options are interpreted first and can be overridden 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
-.B gzip
-warns if it is used.
-This feature will be removed in a future release of
-.BR gzip .
+this feature is supported only for \-\-rsyncable, \-\-synchronous,
+and options like \-9 that set the compression level;
+any other options or operands in
+.B GZIP
+are silently ignored.
 .PP
 You can use an alias or script instead.
-For example, if
+For example, instead of setting GZIP="\-9" in the environment, if
 .B gzip
 is in the directory
 .B /usr/bin
diff --git a/gzip.c b/gzip.c
index a33841a..913fafe 100644
--- a/gzip.c
+++ b/gzip.c
@@ -249,10 +249,6 @@ enum
   PRESUME_INPUT_TTY_OPTION = CHAR_MAX + 1,
   RSYNCABLE_OPTION,
   SYNCHRONOUS_OPTION,
-
-  /* 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";
@@ -277,7 +273,7 @@ static const struct option longopts[] =
     {"-presume-input-tty", no_argument, NULL, PRESUME_INPUT_TTY_OPTION},
     {"quiet",      0, 0, 'q'}, /* quiet mode */
     {"silent",     0, 0, 'q'}, /* quiet mode */
-    {"synchronous",0, 0, SYNCHRONOUS_OPTION},
+    {"synchronous",0, 0, SYNCHRONOUS_OPTION}, /* output data synchronously */
     {"recursive",  0, 0, 'r'}, /* recurse through directories */
     {"suffix",     1, 0, 'S'}, /* use given suffix instead of .gz */
     {"test",       0, 0, 't'}, /* test compressed file integrity */
@@ -430,6 +426,7 @@ int main (int argc, char **argv)
     argv_copy = argv;
     env = add_envopt (&env_argc, &argv_copy, OPTIONS_VAR);
     env_argv = env ? argv_copy : NULL;
+    opterr = !env;
 
 #ifndef GNU_STANDARD
 # define GNU_STANDARD 1
@@ -455,48 +452,35 @@ int main (int argc, char **argv)
 
     while (true) {
         int optc;
-        int longind = -1;
 
         if (env_argv)
           {
-            if (env_argv[optind] && strequ (env_argv[optind], "--"))
-              optc = ENV_OPTION + '-';
-            else
+            /* Get the next option taken from the GZIP environment variable
+               that affects only performance (including compression level).
+               Silently ignore other options, non-options, invalid options,
+               and option syntax errors, so that bad GZIP values do
+               not unduly interfere with normal gzip operation.  */
+            do
               {
                 optc = getopt_long (env_argc, env_argv, shortopts, longopts,
-                                    &longind);
-                if (0 <= optc)
-                  optc += ENV_OPTION;
-                else
+                                    NULL);
+                if (optc < 0)
                   {
-                    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;
+                    opterr = 1;
                     optind = 1;
-                    longind = -1;
+                    break;
                   }
               }
+            while (! (('1' <= optc && optc <= '9')
+                      || optc == RSYNCABLE_OPTION
+                      || optc == SYNCHRONOUS_OPTION));
           }
 
         if (!env_argv)
-          optc = getopt_long (argc, argv, shortopts, longopts, &longind);
+          optc = getopt_long (argc, argv, shortopts, longopts, NULL);
         if (optc < 0)
           break;
 
@@ -532,15 +516,12 @@ 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
@@ -553,7 +534,6 @@ int main (int argc, char **argv)
             break;
 
         case RSYNCABLE_OPTION:
-        case RSYNCABLE_OPTION + ENV_OPTION:
             rsync = 1;
             break;
         case 'S':
@@ -575,7 +555,6 @@ 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 (); finish_out (); break;
@@ -584,28 +563,12 @@ int main (int argc, char **argv)
                     program_name);
             try_help ();
             break;
-        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;
-            FALLTHROUGH;
         case '1':  case '2':  case '3':  case '4':
         case '5':  case '6':  case '7':  case '8':  case '9':
             level = optc - '0';
             break;
 
         default:
-            if (ENV_OPTION <= optc && optc != ENV_OPTION + '?')
-              {
-                /* Output a diagnostic, since getopt_long didn't.  */
-                fprintf (stderr, "%s: ", program_name);
-                if (longind < 0)
-                  fprintf (stderr, "-%c: ", optc - ENV_OPTION);
-                else
-                  fprintf (stderr, "--%s: ", longopts[longind].name);
-                fprintf (stderr, ("option not valid in "OPTIONS_VAR
-                                  " environment variable\n"));
-              }
             try_help ();
         }
     } /* loop on all arguments */
diff --git a/tests/gzip-env b/tests/gzip-env
index b7382d9..44123c9 100755
--- a/tests/gzip-env
+++ b/tests/gzip-env
@@ -1,5 +1,5 @@
 #!/bin/sh
-# Test the obsolescent GZIP environment variable.
+# Test the GZIP environment variable.
 
 # Copyright 2015-2025 Free Software Foundation, Inc.
 
@@ -27,14 +27,18 @@ 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
+  -l --list -L --license -n --no-name -N --name -q --quiet \
+  -r --recursive -Sxxx --suffix=xxx '--suffix xxx' \
+  -t --test -v --verbose -V --version
 do
-  GZIP=$badopt gzip -d <in >out 2>err && fail=1
+  GZIP=$badopt gzip <exp >inagain 2>err || fail=1
+  compare in inagain
+  GZIP=$badopt gzip -d <in >out 2>err || fail=1
+  compare exp out || 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
+for goodopt in \
+  -1 --fast -2 -3 -4 -5 -6 -7 -8 -9 --best --rsyncable --synchronous
 do
   GZIP=$goodopt gzip -d <in >out 2>err || fail=1
   compare exp out || fail=1
diff --git a/znew.in b/znew.in
index b09d967..ad13631 100644
--- a/znew.in
+++ b/znew.in
@@ -55,7 +55,8 @@ new=0
 block=1024
 # block is the disk block size (best guess, need not be exact)
 
-# Beware -s or --suffix in $GZIP.
+# Beware -s or --suffix in $GZIP, which could cause misbehavior
+# in gzip 1.13 and earlier.
 unset GZIP
 ext=.gz
 
-- 
2.45.2