Re: bugs when operating with closed file descriptors

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 7/20/2006 4:49 PM:
> I'm still thinking about how best to patch this.  I know gnulib provides the 
> stdio-safer module (and friends) that guarantee that stdio functions like 
> fopen 
> don't reuse fd's 0, 1, or 2 (and hence that stdin, stdout, and stderr remain 
> closed if they started life closed).  I also know that gnulib provides the 
> closeout module, which we should probably be using (and issue an error if any 
> output was attempted to stdout when it was already closed).

After some hacking in gnulib to provide the necessary hooks for m4's use,
this patch fixes all the listed bugs, and a few others, such as:

$ echo dnl | m4 -tdnl -o/dev/full
$ echo $?
0

Oops - even though we had a write error to the debug stream, we proceeded
as though nothing bad happened.

This patch also pulls in the unlocked-io module - since m4 is
single-threaded, this has the potential of safely speeding up m4 operation
on platforms that provide unlocked variants of I/O functions.

2006-07-24  Eric Blake  <address@hidden>

        Fix bugs related to stream handling.
        * m4/gnulib-cache.m4: Augment with gnulib-tool --import
        unlocked-io stdio-safer stdlib-safer close-stream.
        * configure.ac (AC_CHECK_FUNCS_ONCE): Assume tmpfile; it can be
        provided by gnulib if needed.
        * src/output.c [! HAVE_TMPFILE]: Likewise.
        * src/m4.h (includes): Replace unistd, stdio, and stdlib with
        their safer counterparts.
        (retcode): New global variable.
        * src/input.c (pop_input): Check for read failure.
        * src/freeze.c (reload_frozen_state): Likewise.
        (produce_frozen_state): Check for write failure.
        * src/debug.c (debug_set_file): Likewise.
        * src/m4.c (usage, main): Likewise.
        (retcode): Make global.
        * src/builtin.c (m4_m4exit): Likewise.  Ensure that the exit
        status is non-zero except when everything succeeds.
        * doc/m4.texinfo (M4exit): Document these changes.
        (Incompatibilities): Remove documentation of bug now fixed.
        * NEWS: Document these fixes.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFExSag84KuGfSFAYARAk4jAJ9HR9YbfwSNo2ggX+aTAP86W60k8wCfQEaa
nb5kRwHKrTZx4jVtURziLgU=
=EDOE
-----END PGP SIGNATURE-----

Index: NEWS
===================================================================
RCS file: /sources/m4/m4/NEWS,v
retrieving revision 1.1.1.1.2.42
diff -u -p -r1.1.1.1.2.42 NEWS
--- NEWS        19 Jul 2006 14:55:52 -0000      1.1.1.1.2.42
+++ NEWS        24 Jul 2006 19:51:50 -0000
@@ -9,6 +9,9 @@ Version 1.4.6 - ?? 2006, by ??  (CVS ver
 * The format macro now understands %F, %g, and %G.
 * When loading frozen files, m4 now exits with status 63 if version
   mismatch is detected.
+* Fix bugs that occurred when invoked with stdout or stderr closed.  Detect
+  write failures to stdout.
+* The m4exit macro now converts values outside the range 0-255 to 1.
 
 Version 1.4.5 - 15 July 2006, by Eric Blake  (CVS version 1.4.4c)
 
Index: configure.ac
===================================================================
RCS file: /sources/m4/m4/configure.ac,v
retrieving revision 1.36.2.23
diff -u -p -r1.36.2.23 configure.ac
--- configure.ac        17 Jul 2006 16:35:12 -0000      1.36.2.23
+++ configure.ac        24 Jul 2006 19:51:50 -0000
@@ -48,7 +48,7 @@ AC_CHECK_MEMBERS([struct sigaction.sa_si
 AC_TYPE_SIGNAL
 AC_TYPE_SIZE_T
 
-AC_CHECK_FUNCS_ONCE([sigaction sigaltstack sigstack sigvec strerror tmpfile])
+AC_CHECK_FUNCS_ONCE([sigaction sigaltstack sigstack sigvec strerror])
 
 M4_INIT
 
Index: doc/m4.texinfo
===================================================================
RCS file: /sources/m4/m4/doc/m4.texinfo,v
retrieving revision 1.1.1.1.2.51
diff -u -p -r1.1.1.1.2.51 m4.texinfo
--- doc/m4.texinfo      19 Jul 2006 18:16:23 -0000      1.1.1.1.2.51
+++ doc/m4.texinfo      24 Jul 2006 19:51:51 -0000
@@ -3873,8 +3873,9 @@ read, you can use @code{m4exit}:
 
 @deffn Builtin m4exit (@dvar{code, 0})
 Causes @code{m4} to exit, with exit status @var{code}.  If @var{code} is
-left out, the exit status is zero.  No further input is read, and all
-wrapped and diverted text is discarded.
+left out, the exit status is zero.  If @var{code} cannot be parsed, or
+is outside the range of 0 to 255, the exit status is one.  No further
+input is read, and all wrapped and diverted text is discarded.
 @end deffn
 
 A common use of this is to abort processing:
@@ -3908,6 +3909,11 @@ divert
 m4exit
 @end example
 
+Note that it is still possible for the exit status to be different than
+what was requested by @code{m4exit}.  If @code{m4} detects some other
+error, such as a write error on standard out, the exit status will be
+non-zero even if @code{m4exit} requested zero.
+
 @node Frozen files
 @chapter Fast loading of frozen state
 
@@ -4235,11 +4241,6 @@ implemented for the various builtins tha
 @code{m4exit} (@pxref{M4exit}) with a non-numeric argument).
 
 @item
address@hidden requires an application to exit with non-zero status if
-it encounters a read error on stdin or write error on stdout, but GNU
address@hidden still exits with status 0 even if the disk is full.
-
address@hidden
 @acronym{POSIX} requires @code{m4wrap} (@pxref{M4wrap}) to act in FIFO
 (first-in, first-out) order, but GNU @code{m4} currently uses LIFO order.
 Furthermore, @acronym{POSIX} states that only the first argument to
Index: m4/gnulib-cache.m4
===================================================================
RCS file: /sources/m4/m4/m4/Attic/gnulib-cache.m4,v
retrieving revision 1.1.2.7
diff -u -p -r1.1.2.7 gnulib-cache.m4
--- m4/gnulib-cache.m4  20 Jul 2006 15:41:00 -0000      1.1.2.7
+++ m4/gnulib-cache.m4  24 Jul 2006 19:51:51 -0000
@@ -15,10 +15,10 @@
 
 
 # Specification in the form of a command-line invocation:
-#   gnulib-tool --import --dir=. --lib=libm4 --source-base=lib --m4-base=m4 
--doc-base=doc --aux-dir=. --macro-prefix=M4 --assume-autoconf=2.60 alloca 
binary-io error fdl gendocs getopt mkstemp obstack regex strtol xalloc 
xvasprintf
+#   gnulib-tool --import --dir=. --lib=libm4 --source-base=lib --m4-base=m4 
--doc-base=doc --aux-dir=. --macro-prefix=M4 --assume-autoconf=2.60 alloca 
binary-io close-stream error fdl gendocs getopt mkstemp obstack regex 
stdio-safer stdlib-safer strtol unlocked-io xalloc xvasprintf
 
 # Specification in the form of a few gnulib-tool.m4 macro invocations:
-gl_MODULES([alloca binary-io error fdl gendocs getopt mkstemp obstack regex 
strtol xalloc xvasprintf])
+gl_MODULES([alloca binary-io close-stream error fdl gendocs getopt mkstemp 
obstack regex stdio-safer stdlib-safer strtol unlocked-io xalloc xvasprintf])
 gl_AVOID([])
 gl_SOURCE_BASE([lib])
 gl_M4_BASE([m4])
Index: src/builtin.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/builtin.c,v
retrieving revision 1.1.1.1.2.26
diff -u -p -r1.1.1.1.2.26 builtin.c
--- src/builtin.c       17 Jul 2006 16:35:12 -0000      1.1.1.1.2.26
+++ src/builtin.c       24 Jul 2006 19:51:51 -0000
@@ -1257,13 +1257,29 @@ m4___line__ (struct obstack *obs, int ar
 static void
 m4_m4exit (struct obstack *obs, int argc, token_data **argv)
 {
-  int exit_code = 0;
-
-  if (bad_argc (argv[0], argc, 1, 2))
-    return;
-  if (argc >= 2  && !numeric_arg (argv[0], ARG (1), &exit_code))
-    exit_code = 0;
+  int exit_code = EXIT_SUCCESS;
 
+  /* Warn on bad arguments, but still exit.  */
+  bad_argc (argv[0], argc, 1, 2);
+  if (argc >= 2 && !numeric_arg (argv[0], ARG (1), &exit_code))
+    exit_code = EXIT_FAILURE;
+  if (exit_code < 0 || exit_code > 255)
+    {
+      M4ERROR ((warning_status, 0,
+                "exit status out of range: `%d'", exit_code));
+      exit_code = EXIT_FAILURE;
+    }
+  if (close_stream (stdout) != 0)
+    {
+      M4ERROR ((warning_status, errno, "write error"));
+      if (exit_code == 0)
+        exit_code = EXIT_FAILURE;
+    }
+  /* Change debug stream back to stderr, to force flushing debug stream and
+     detect any errors it might have encountered.  */
+  debug_set_output (NULL);
+  if (exit_code == 0 && retcode != 0)
+    exit_code = retcode;
   exit (exit_code);
 }
 
Index: src/debug.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/debug.c,v
retrieving revision 1.1.1.1.2.4
diff -u -p -r1.1.1.1.2.4 debug.c
--- src/debug.c 30 Jun 2006 18:58:12 -0000      1.1.1.1.2.4
+++ src/debug.c 24 Jul 2006 19:51:52 -0000
@@ -137,8 +137,12 @@ debug_set_file (FILE *fp)
 {
   struct stat stdout_stat, debug_stat;
 
-  if (debug != NULL && debug != stderr && debug != stdout)
-    fclose (debug);
+  if (debug != NULL && debug != stderr && debug != stdout
+      && close_stream (debug) != 0)
+    {
+      M4ERROR ((warning_status, errno, "error writing to debug stream"));
+      retcode = EXIT_FAILURE;
+    }
   debug = fp;
 
   if (debug != NULL && debug != stdout)
@@ -154,8 +158,12 @@ debug_set_file (FILE *fp)
          && stdout_stat.st_dev == debug_stat.st_dev
          && stdout_stat.st_ino != 0)
        {
-         if (debug != stderr)
-           fclose (debug);
+         if (debug != stderr && close_stream (debug) != 0)
+            {
+              M4ERROR ((warning_status, errno,
+                        "error writing to debug stream"));
+              retcode = EXIT_FAILURE;
+            }
          debug = stdout;
        }
     }
Index: src/freeze.c
===================================================================
RCS file: /sources/m4/m4/src/freeze.c,v
retrieving revision 1.1.1.1.2.10
diff -u -p -r1.1.1.1.2.10 freeze.c
--- src/freeze.c        19 Jul 2006 14:55:53 -0000      1.1.1.1.2.10
+++ src/freeze.c        24 Jul 2006 19:51:52 -0000
@@ -148,7 +148,8 @@ INTERNAL ERROR: bad token data type in f
   /* All done.  */
 
   fputs ("# End of frozen state file\n", file);
-  fclose (file);
+  if (close_stream (file) != 0)
+    M4ERROR ((EXIT_FAILURE, errno, "unable to create frozen state"));
 }
 
 /*----------------------------------------------------------------------.
@@ -367,7 +368,9 @@ reload_frozen_state (const char *name)
 
   free (string[0]);
   free (string[1]);
-  fclose (file);
+  errno = 0;
+  if (ferror (file) || fclose (file) != 0)
+    M4ERROR ((EXIT_FAILURE, errno, "unable to read frozen state"));
 
 #undef GET_CHARACTER
 #undef GET_DIRECTIVE
Index: src/input.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/input.c,v
retrieving revision 1.1.1.1.2.9
diff -u -p -r1.1.1.1.2.9 input.c
--- src/input.c 13 Jul 2006 22:09:54 -0000      1.1.1.1.2.9
+++ src/input.c 24 Jul 2006 19:51:52 -0000
@@ -307,7 +307,17 @@ pop_input (void)
        DEBUG_MESSAGE2 ("input reverted to %s, line %d",
                        isp->u.u_f.name, isp->u.u_f.lineno);
 
-      fclose (isp->u.u_f.file);
+      if (ferror (isp->u.u_f.file))
+        {
+          M4ERROR ((warning_status, 0, "read error"));
+          fclose (isp->u.u_f.file);
+          retcode = EXIT_FAILURE;
+        }
+      else if (fclose (isp->u.u_f.file) == EOF)
+        {
+          M4ERROR ((warning_status, errno, "error reading file"));
+          retcode = EXIT_FAILURE;
+        }
       current_file = isp->u.u_f.name;
       current_line = isp->u.u_f.lineno;
       output_current_line = isp->u.u_f.out_lineno;
Index: src/m4.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/m4.c,v
retrieving revision 1.1.1.1.2.19
diff -u -p -r1.1.1.1.2.19 m4.c
--- src/m4.c    19 Jul 2006 14:55:53 -0000      1.1.1.1.2.19
+++ src/m4.c    24 Jul 2006 19:51:52 -0000
@@ -24,7 +24,9 @@
 #include <getopt.h>
 #include <signal.h>
 
-static void usage _((int));
+#include "close-stream.h"
+
+static void usage (int);
 
 /* Operate interactively (-e).  */
 static int interactive = 0;
@@ -221,6 +223,9 @@ Exit status is 0 for success, 1 for fail
 mismatch, or whatever value was passed to the m4exit macro.\n\
 ", stdout);
     }
+
+  if (close_stream (stdout) != 0)
+    M4ERROR ((EXIT_FAILURE, errno, "write error"));
   exit (status);
 }
 
@@ -259,6 +264,10 @@ static const struct option long_options[
   { 0, 0, 0, 0 },
 };
 
+/* Global catchall for any errors that should affect final error status, but
+   where we try to continue execution in the meantime.  */
+int retcode;
+
 #ifdef ENABLE_CHANGEWORD
 #define OPTSTRING "B:D:EF:GH:I:L:N:PQR:S:T:U:W:d::el:o:st:"
 #else
@@ -268,7 +277,6 @@ static const struct option long_options[
 int
 main (int argc, char *const *argv, char *const *envp)
 {
-  int retcode = EXIT_SUCCESS;
   macro_definition *head;      /* head of deferred argument list */
   macro_definition *tail;
   macro_definition *new;
@@ -278,6 +286,7 @@ main (int argc, char *const *argv, char 
   FILE *fp;
 
   program_name = argv[0];
+  retcode = EXIT_SUCCESS;
 
   include_init ();
   debug_init ();
@@ -408,6 +417,8 @@ This is free software; see the source fo
 warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\
 ");
 
+      if (close_stream (stdout) != 0)
+       M4ERROR ((EXIT_FAILURE, errno, "write error"));
       exit (EXIT_SUCCESS);
     }
 
@@ -512,6 +523,10 @@ warranty; not even for MERCHANTABILITY o
   while (pop_wrapup ())
     expand_input ();
 
+  /* Change debug stream back to stderr, to force flushing debug stream and
+     detect any errors it might have encountered.  */
+  debug_set_output (NULL);
+
   if (frozen_file_to_write)
     produce_frozen_state (frozen_file_to_write);
   else
@@ -520,5 +535,7 @@ warranty; not even for MERCHANTABILITY o
       undivert_all ();
     }
 
+  if (close_stream (stdout) != 0)
+    M4ERROR ((EXIT_FAILURE, errno, "write error"));
   exit (retcode);
 }
Index: src/m4.h
===================================================================
RCS file: /sources/m4/m4/src/m4.h,v
retrieving revision 1.1.1.1.2.18
diff -u -p -r1.1.1.1.2.18 m4.h
--- src/m4.h    21 Jul 2006 13:22:03 -0000      1.1.1.1.2.18
+++ src/m4.h    24 Jul 2006 19:51:52 -0000
@@ -48,21 +48,19 @@
 
 #include <ctype.h>
 #include <errno.h>
-#include <stdio.h>
-#include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
 
 #include "binary-io.h"
+#include "close-stream.h"
 #include "error.h"
 #include "exit.h"
 #include "obstack.h"
+#include "stdio--.h"
+#include "stdlib--.h"
+#include "unistd--.h"
 #include "xalloc.h"
 
-#ifdef HAVE_UNISTD_H
-# include <unistd.h>
-#endif
-
 /* If FALSE is defined, we presume TRUE is defined too.  In this case,
    merely typedef boolean as being int.  Or else, define these all.  */
 #ifndef FALSE
@@ -114,6 +112,7 @@ extern const char *user_word_regexp;        /* 
 #endif
 
 /* Error handling.  */
+extern int retcode;
 #define M4ERROR(Arglist) \
   (reference_error (), error Arglist)
 
Index: src/output.c
===================================================================
RCS file: /sources/m4/m4/src/Attic/output.c,v
retrieving revision 1.1.1.1.2.7
diff -u -p -r1.1.1.1.2.7 output.c
--- src/output.c        10 Jul 2006 01:44:10 -0000      1.1.1.1.2.7
+++ src/output.c        24 Jul 2006 19:51:52 -0000
@@ -34,10 +34,6 @@
 /* Size of buffer size to use while copying files.  */
 #define COPY_BUFFER_SIZE (32 * 512)
 
-#ifdef HAVE_TMPFILE
-extern FILE *tmpfile ();
-#endif
-
 /* Output functions.  Most of the complexity is for handling cpp like
    sync lines.
 
@@ -103,27 +99,6 @@ output_init (void)
   output_unused = 0;
 }
 
-#ifndef HAVE_TMPFILE
-
-/* Implement tmpfile(3) for non-USG systems.  */
-
-static FILE *
-tmpfile (void)
-{
-  char buf[32];
-  int fd;
-
-  strcpy (buf, "/tmp/m4XXXXXX");
-  fd = mkstemp (buf);
-  if (fd < 0)
-    return NULL;
-
-  unlink (buf);
-  return fdopen (fd, "w+");
-}
-
-#endif /* not HAVE_TMPFILE */
-
 /*-----------------------------------------------------------------------.
 | Reorganize in-memory diversion buffers so the current diversion can   |
 | accomodate LENGTH more characters without further reorganization.  The |