Re: RFC PATCH 1/2] AS_INIT: try to ensure fds 0, 1, 2 are open

[Zack Weinberg]

On Thu, Aug 27, 2020 at 6:49 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 8/27/20 3:20 PM, Zack Weinberg wrote:
> > On Thu, Aug 27, 2020 at 2:09 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> >>
> >>> +  if (exec 3>&0) 2>/dev/null; then :; else exec 0</dev/null; fi
> >>> +  if (exec 3>&1) 2>/dev/null; then :; else exec 1>/dev/null; fi
> >>> +  if (exec 3>&2)            ; then :; else exec 2>/dev/null; fi
> >>
> >> This is all _AS_ENSURE_STANDARD_FDS needs to do; none of the other stuff is
> >> needed and I suggest omitting it as a maintenance time-sink.
> >
> > I can definitely see how the rest of it could turn into a maintenance
> > time sink, but are you sure we don't need a workaround of any kind for
> > this construct succeeding even though the target fd is closed, in very
> > old shells?
>
> Buggy old shells likely won't benefit from the other stuff anyway, as they'll 
> be
> running in older environments where the other stuff likely doesn't work 
> anyway.
> So the other stuff has little benefit to counterbalance the maintenance 
> timesink.

Fair enough.  Also I checked and _AS_DETECT_BETTER_SHELL works fine with
the standard fds closed and will reject all shells this old anyway
(they don't have
support for shell functions).

I pushed the revised patch below.

----

A patch was recently proposed for GNU libc to make *all* processes
start up with file descriptors 0, 1, and 2 guaranteed to be open.
Part of the rationale for this patch was that configure scripts fail
catastrophically if these fds are closed, even if you just want to run
--help or --version, e.g.

   $ ./configure --version <&-; echo $?
   ./configure: line 555: 0: Bad file descriptor
   1

configure scripts cannot rely on behavior specific to GNU libc, so
whether or not that patch gets committed, it makes sense for us to
make configure scripts robust against being started up with closed
stdin/stdout/stderr.

This patch adds code to ensure fds 0, 1, and 2 are open, early in
_AS_SHELL_SANITIZE.  It uses a construct, ‘(exec 3>&n)’, that’s known
not to work in very old shells, but that’s OK because those shells
will be rejected by _AS_DETECT_BETTER_SHELL anyway.  The worst-case
scenario is that the “This script requires a shell more modern than
all the shells I found on your system” error message won’t get printed.

When these fds are found not to be open, we open them on /dev/null, in
the normal I/O direction (0 for reading, 1 and 2 for writing).  There
is a case for opening them in the *opposite* direction so that, for
instance, writes to fd 1 will fail when fd 1 started out closed.
However, that would expose latent bugs that I think should be dealt
with *after* 2.70.  (See Savannah bug #110300 for more detail.)

I also took the opportunity to rationalize the order of operations in
_AS_SHELL_SANITIZE a little.  All the special shell and environment
variables that we care about are dealt with immediately after
AS_BOURNE_COMPATIBLE, and _AS_PATH_SEPARATOR_PREPARE happens
immediately before the first use of _AS_PATH_WALK.

* lib/m4sugar/m4sh.m4 (_AS_ENSURE_STANDARD_FDS): New macro.
  (_AS_SHELL_SANITIZE): Move the “Unset variables that we do not need”
  and “NLS nuisances” blocks immediately after setting IFS; merge the
  unsetting of CDPATH into the main unsetting loop; move invocation of
  _AS_PATH_SEPARATOR_PREPARE to immediately above the “Find who we are”
  block; invoke _AS_ENSURE_STANDARD_FDS immediately before
  _AS_PATH_SEPARATOR_PREPARE.

* tests/base.at (configure with closed standard fds): New test.
* tests/torture.at (--help and --version in unwritable directory): New test.
---
 lib/m4sugar/m4sh.m4 | 78 ++++++++++++++++++++++++++++++---------------
 tests/base.at       | 73 ++++++++++++++++++++++++++++++++++++++++++
 tests/torture.at    | 27 ++++++++++++++++
 3 files changed, 152 insertions(+), 26 deletions(-)

diff --git a/lib/m4sugar/m4sh.m4 b/lib/m4sugar/m4sh.m4
index c9e86246..bd4d2b9e 100644
--- a/lib/m4sugar/m4sh.m4
+++ b/lib/m4sugar/m4sh.m4
@@ -300,6 +300,28 @@ dnl We shouldn't have to worry about any traps
being active at this point.
 exit 255])# _AS_REEXEC_WITH_SHELL


+# _AS_ENSURE_STANDARD_FDS
+# -----------------------
+# Ensure that file descriptors 0, 1, and 2 are open, as a defensive
+# measure against weird environments that run configure scripts
+# with these descriptors closed.
+# `exec m>&n` fails in POSIX sh when fd N is closed, but succeeds
+# regardless of whether fd N is open in some old shells, e.g. Solaris
+# /bin/sh.  This is OK because those shells will be rejected by
+# _AS_DETECT_BETTER_SHELL anyway.
+# TODO post-2.70: use "backward" redirections when opening these fds,
+# so we preserve their unusable state.  Needs downstream logic to
+# stop on the first failed attempt to write to fd 1 or 2, so we don't
+# run through an entire configure script spewing "write error"
+# messages when fd 1 is closed.
+m4_defun([_AS_ENSURE_STANDARD_FDS], [dnl
+# Ensure that fds 0, 1, and 2 are open.
+if (exec 3>&0) 2>/dev/null; then :; else exec 0</dev/null; fi
+if (exec 3>&1) 2>/dev/null; then :; else exec 1>/dev/null; fi
+if (exec 3>&2)            ; then :; else exec 2>/dev/null; fi
+])
+
+
 # _AS_PREPARE
 # -----------
 # This macro has a very special status.  Normal use of M4sh relies
@@ -456,18 +478,42 @@ m4_defun([_AS_SHELL_SANITIZE],
 [m4_text_box([M4sh Initialization.])

 AS_BOURNE_COMPATIBLE
-_AS_PATH_SEPARATOR_PREPARE

-# IFS
-# We need space, tab and new line, in precisely that order.  Quoting is
-# there to prevent editors from complaining about space-tab.
-# (If _AS_PATH_WALK were called with IFS unset, it would disable word
-# splitting by setting IFS to empty value.)
+# Reset variables that may have inherited troublesome values from
+# the environment.
+
+# IFS needs to be set, to space, tab, and newline, in precisely that order.
+# (If _AS_PATH_WALK were called with IFS unset, it would have the
+# side effect of setting IFS to empty, thus disabling word splitting.)
+# Quoting is to prevent editors from complaining about space-tab.
 as_nl='
 '
 export as_nl
 IFS=" ""    $as_nl"

+PS1='$ '
+PS2='> '
+PS4='+ '
+
+# Ensure predictable behavior from utilities with locale-dependent output.
+LC_ALL=C
+export LC_ALL
+LANGUAGE=C
+export LANGUAGE
+
+# We cannot yet rely on "unset" to work, but we need these variables
+# to be unset--not just set to an empty or harmless value--now, to
+# avoid bugs in old shells (e.g. pre-3.0 UWIN ksh).  This construct
+# also avoids known problems related to "unset" and subshell syntax
+# in other old shells (e.g. bash 2.01 and pdksh 5.2.14).
+for as_var in BASH_ENV ENV MAIL MAILPATH CDPATH
+do eval test \${$as_var+y} \
+  && ( (unset $as_var) || exit 1) >/dev/null 2>&1 && unset $as_var || :
+done
+
+_AS_ENSURE_STANDARD_FDS
+_AS_PATH_SEPARATOR_PREPARE
+
 # Find who we are.  Look in the path if we contain no directory separator.
 as_myself=
 case $[0] in @%:@((
@@ -486,26 +532,6 @@ if test ! -f "$as_myself"; then
   AS_EXIT
 fi

-# Unset variables that we do not need and which cause bugs (e.g. in
-# pre-3.0 UWIN ksh).  But do not cause bugs in bash 2.01; the "|| exit 1"
-# suppresses any "Segmentation fault" message there.  '((' could
-# trigger a bug in pdksh 5.2.14.
-for as_var in BASH_ENV ENV MAIL MAILPATH
-do eval test \${$as_var+y} \
-  && ( (unset $as_var) || exit 1) >/dev/null 2>&1 && unset $as_var || :
-done
-PS1='$ '
-PS2='> '
-PS4='+ '
-
-# NLS nuisances.
-LC_ALL=C
-export LC_ALL
-LANGUAGE=C
-export LANGUAGE
-
-# CDPATH.
-(unset CDPATH) >/dev/null 2>&1 && unset CDPATH
 _m4_popdef([AS_EXIT])])# _AS_SHELL_SANITIZE


diff --git a/tests/base.at b/tests/base.at
index 4042a8aa..6894990a 100644
--- a/tests/base.at
+++ b/tests/base.at
@@ -846,3 +846,76 @@ FOO
 ]])

 AT_CLEANUP
+
+## ----------------------------------- ##
+## configure with closed standard fds  ##
+## ----------------------------------- ##
+
+AT_SETUP([configure with closed standard fds])
+AT_KEYWORDS([AS@&t@_INIT])
+
+# Create a configure script that runs a relatively complicated but
+# self-contained test.  Run it.
+AT_CONFIGURE_AC([[AC_PROG_CC]])
+AT_CHECK_AUTOCONF
+AT_CHECK_AUTOHEADER
+AT_CHECK_CONFIGURE([], [], [stdout], [stderr])
+AT_CHECK_ENV
+
+mv stdout stdout-expected
+mv stderr stderr-expected
+mv state-env.after state-env-expected
+mv config.status config-status-expected
+mv config.h config-h-expected
+
+# Run it again with stdin (fd 0) closed.
+# There should be no change to the stdout or stderr output and thoe
+# result of configuration should be the same.
+
+AT_CHECK_CONFIGURE([ 0<&- ], [], [stdout], [stderr])
+AT_CHECK_ENV
+AT_CMP([stdout-expected], [stdout])
+AT_CMP([stderr-expected], [stderr])
+AT_CONFIG_CMP([state-env-expected], [state-env.after])
+
+mv stdout stdout-closed-0
+mv stderr stderr-closed-0
+mv state-env.after state-env-closed-0
+mv config.status config-status-closed-0
+mv config.h config-h-closed-0
+
+# Run it again with stdout (fd 1) closed.
+# There should be no change to the stderr output and the
+# result of configuration should be the same.  (Any output
+# that would have gone to stdout, of course, is lost.)
+
+AT_CHECK_CONFIGURE([ 1>&- ], [], [stdout], [stderr])
+AT_CHECK_ENV
+AT_CHECK([test -f stdout && test ! -s stdout])
+AT_CMP([stderr-expected], [stderr])
+AT_CONFIG_CMP([state-env-expected], [state-env.after])
+
+mv stdout stdout-closed-1
+mv stderr stderr-closed-1
+mv state-env.after state-env-closed-1
+mv config.status config-status-closed-1
+mv config.h config-h-closed-1
+
+# Run it again with stderr (fd 2) closed.
+# There should be no change to the stdout output and the
+# result of configuration should be the same.  (Any output
+# that would have gone to stderr, of course, is lost.)
+
+AT_CHECK_CONFIGURE([ 2>&- ], [], [stdout], [stderr])
+AT_CHECK_ENV
+AT_CMP([stdout-expected], [stdout])
+AT_CHECK([test -f stderr && test ! -s stderr])
+AT_CONFIG_CMP([state-env-expected], [state-env.after])
+
+mv stdout stdout-closed-2
+mv stderr stderr-closed-2
+mv state-env.after state-env-closed-2
+mv config.status config-status-closed-2
+mv config.h config-h-closed-2
+
+AT_CLEANUP
diff --git a/tests/torture.at b/tests/torture.at
index 616e051c..53859f6b 100644
--- a/tests/torture.at
+++ b/tests/torture.at
@@ -493,6 +493,33 @@ AT_CLEANUP



+## ---------------------- ##
+## --help and --version.  ##
+## ---------------------- ##
+
+AT_SETUP([--help and --version in unwritable directory])
+
+AS_MKDIR_P([inner])
+AT_DATA([inner/configure.ac],
+[[AC_INIT
+AC_OUTPUT
+]])
+
+AT_CHECK_M4([(cd inner && autoconf --force)])
+AT_CHECK([test -s inner/configure])
+if test "$SHELL_N" != none; then
+  AT_CHECK_SHELL_SYNTAX([inner/configure])
+fi
+
+chmod a-w inner
+AT_CHECK([(cd inner && ./configure --help)], 0, [ignore], [ignore])
+AT_CHECK([(cd inner && ./configure --version)], 0, [ignore], [ignore])
+chmod u+w inner
+
+AT_CLEANUP
+
+
+
 ## -------------------------------------------- ##
 ## Check that `#define' templates are honored.  ##
 ## -------------------------------------------- ##
-- 
2.28.0