Re: freopen_safer

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> This should definitely be fixed for clients of the stdio-safer module (and 
its 
> corresponding "stdio--.h") - if we ever violate the premise that 
> STDERR_FILENO==fileno(stderr), then the whole point of the *_safer functions 
is 
> lost.

Here's what I will probably be committing soon.


From: Eric Blake <address@hidden>
Date: Thu, 5 Nov 2009 15:13:00 -0700
Subject: [PATCH] freopen-safer: new module

* modules/freopen-safer: New module.
* m4/stdio-safer.m4 (gl_FREOPEN_SAFER): New macro.
* lib/freopen-safer.c (freopen_safer): New file.
* lib/stdio-safer.h (freopen_safer): New declaration.
* lib/stdio--.h (freopen): New override.
* MODULES.html.sh (File stream based Input/Output): Mention it.
* doc/posix-functions/freopen.texi (freopen): Mention pitfalls and
freopen-safer module.
* doc/posix-functions/stderr.texi (stderr): Likewise.
* doc/posix-functions/stdin.texi (stdin): Likewise.
* doc/posix-functions/stdout.texi (stdout): Likewise.
* modules/freopen-safer-tests: New test.
* tests/test-reopen-safer.c: New file.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                        |   17 ++++++
 MODULES.html.sh                  |    1 +
 doc/posix-functions/freopen.texi |    5 ++
 doc/posix-functions/stderr.texi  |    7 +++
 doc/posix-functions/stdin.texi   |    7 +++
 doc/posix-functions/stdout.texi  |    7 +++
 lib/freopen-safer.c              |  103 ++++++++++++++++++++++++++++++++++++
 lib/stdio--.h                    |    5 ++
 lib/stdio-safer.h                |   12 ++++
 m4/stdio-safer.m4                |    7 ++-
 modules/freopen-safer            |   29 ++++++++++
 modules/freopen-safer-tests      |   10 ++++
 tests/test-freopen-safer.c       |  107 ++++++++++++++++++++++++++++++++++++++
 13 files changed, 316 insertions(+), 1 deletions(-)
 create mode 100644 lib/freopen-safer.c
 create mode 100644 modules/freopen-safer
 create mode 100644 modules/freopen-safer-tests
 create mode 100644 tests/test-freopen-safer.c

diff --git a/ChangeLog b/ChangeLog
index 7e9d945..1635de2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2009-11-05  Eric Blake  <address@hidden>
+
+       freopen-safer: new module
+       * modules/freopen-safer: New module.
+       * m4/stdio-safer.m4 (gl_FREOPEN_SAFER): New macro.
+       * lib/freopen-safer.c (freopen_safer): New file.
+       * lib/stdio-safer.h (freopen_safer): New declaration.
+       * lib/stdio--.h (freopen): New override.
+       * MODULES.html.sh (File stream based Input/Output): Mention it.
+       * doc/posix-functions/freopen.texi (freopen): Mention pitfalls and
+       freopen-safer module.
+       * doc/posix-functions/stderr.texi (stderr): Likewise.
+       * doc/posix-functions/stdin.texi (stdin): Likewise.
+       * doc/posix-functions/stdout.texi (stdout): Likewise.
+       * modules/freopen-safer-tests: New test.
+       * tests/test-reopen-safer.c: New file.
+
 2009-11-05  Jim Meyering  <address@hidden>

        maint.mk: Prohibit inclusion of "close-stream.h" without use.
diff --git a/MODULES.html.sh b/MODULES.html.sh
index 23a3ddc..cd8eef2 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -2558,6 +2558,7 @@ func_all_modules ()
   func_module freading
   func_module freadptr
   func_module freadseek
+  func_module freopen-safer
   func_module fwritable
   func_module fwriting
   func_module getpass
diff --git a/doc/posix-functions/freopen.texi b/doc/posix-functions/freopen.texi
index b446823..c8b60c2 100644
--- a/doc/posix-functions/freopen.texi
+++ b/doc/posix-functions/freopen.texi
@@ -23,4 +23,9 @@ freopen
 and (without the slash) names a nonexistent file or a file that is not a
 directory, on some platforms:
 HP-UX 11.00, Solaris 9, Irix 5.3.
address@hidden
+Applications should not assume that @code{fileno(f)} will be the same
+before and after a call to @code{freopen(name,mode,f)}.  However, the
+module freopen-safer can at least protect @code{stdin}, @code{stdout},
+and @code{stderr}.
 @end itemize
diff --git a/doc/posix-functions/stderr.texi b/doc/posix-functions/stderr.texi
index 5a518a6..7b8c17f 100644
--- a/doc/posix-functions/stderr.texi
+++ b/doc/posix-functions/stderr.texi
@@ -17,4 +17,11 @@ stderr
 One workaround is to use freopen(NULL, ``r+'', stderr) on Cygwin 1.5.21
 or newer.  Another is to use the gnulib ftello module and do
 ftello(stderr).
address@hidden
+POSIX states that a setuid application can guarantee that fd 2 is
+open, but some systems guarantee this even for non-setuid programs.
+If an application is executed with fd 2 closed, use of @code{stderr}
+can affect an unrelated file that happened to be assigned to fd 2.
+The gnulib *-safer modules may be used to guarantee that fd 2 stays
+reserved for @code{stderr}.
 @end itemize
diff --git a/doc/posix-functions/stdin.texi b/doc/posix-functions/stdin.texi
index dfa13ec..0283542 100644
--- a/doc/posix-functions/stdin.texi
+++ b/doc/posix-functions/stdin.texi
@@ -17,4 +17,11 @@ stdin
 One workaround is to use freopen(NULL, ``r'', stdin) on Cygwin 1.5.21
 or newer.  Another is to use the gnulib ftello module and do
 ftello(stdin).
address@hidden
+POSIX states that a setuid application can guarantee that fd 0 is
+open, but some systems guarantee this even for non-setuid programs.
+If an application is executed with fd 0 closed, use of @code{stdin}
+can affect an unrelated file that happened to be assigned to fd 0.
+The gnulib *-safer modules may be used to guarantee that fd 0 stays
+reserved for @code{stdin}.
 @end itemize
diff --git a/doc/posix-functions/stdout.texi b/doc/posix-functions/stdout.texi
index bdeb977..23a0e96 100644
--- a/doc/posix-functions/stdout.texi
+++ b/doc/posix-functions/stdout.texi
@@ -17,4 +17,11 @@ stdout
 One workaround is to use freopen(NULL, ``w'', stdout) on Cygwin 1.5.21
 or newer.  Another is to use the gnulib ftello module and do
 ftello(stdout).
address@hidden
+POSIX states that a setuid application can guarantee that fd 1 is
+open, but some systems guarantee this even for non-setuid programs.
+If an application is executed with fd 1 closed, use of @code{stdout}
+can affect an unrelated file that happened to be assigned to fd 1.
+The gnulib *-safer modules may be used to guarantee that fd 1 stays
+reserved for @code{stdout}.
 @end itemize
diff --git a/lib/freopen-safer.c b/lib/freopen-safer.c
new file mode 100644
index 0000000..5a8dc91
--- /dev/null
+++ b/lib/freopen-safer.c
@@ -0,0 +1,103 @@
+/* Invoke freopen, but avoid some glitches.
+
+   Copyright (C) 2009 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/>.  */
+
+/* Written by Eric Blake.  */
+
+#include <config.h>
+
+#include "stdio-safer.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <unistd.h>
+
+/* Guarantee that FD is open; all smaller FDs must already be open.
+   Return true if successful.  */
+static bool
+protect_fd (int fd)
+{
+  int value = open ("/dev/null", O_RDONLY);
+  if (value != fd)
+    {
+      if (0 <= value)
+        {
+          close (value);
+          errno = EBADF; /* Unexpected; this is as good as anything else.  */
+        }
+      return false;
+    }
+  return true;
+}
+
+/* Like freopen, but guarantee that reopening stdin, stdout, or stderr
+   preserves the invariant that STDxxx_FILENO==fileno(stdxxx), and
+   that no other stream will interfere with the standard streams.
+   This is necessary because most freopen implementations will change
+   the associated fd of a stream to the lowest available slot.  */
+
+FILE *
+freopen_safer (char const *name, char const *mode, FILE *f)
+{
+  /* Unfortunately, we cannot use the fopen_safer approach of using
+     fdopen (dup_safer (fileno (freopen (cmd, mode, f)))), because we
+     need to return f itself.  The implementation of freopen(NULL,m,f)
+     is system-dependent, so the best we can do is guarantee that all
+     lower-valued standard fds are open prior to the freopen call,
+     even though this puts more pressure on open fds.  */
+  bool protect_in = false;
+  bool protect_out = false;
+  bool protect_err = false;
+  int saved_errno;
+
+  switch (fileno (f))
+    {
+    default: /* -1 or not a standard stream.  */
+      if (dup2 (STDERR_FILENO, STDERR_FILENO) != STDERR_FILENO)
+        protect_err = true;
+      /* fall through */
+    case STDERR_FILENO:
+      if (dup2 (STDOUT_FILENO, STDOUT_FILENO) != STDOUT_FILENO)
+        protect_out = true;
+      /* fall through */
+    case STDOUT_FILENO:
+      if (dup2 (STDIN_FILENO, STDIN_FILENO) != STDIN_FILENO)
+        protect_in = true;
+      /* fall through */
+    case STDIN_FILENO:
+      /* Nothing left to protect.  */
+      break;
+    }
+  if (protect_in && !protect_fd (STDIN_FILENO))
+    f = NULL;
+  else if (protect_out && !protect_fd (STDOUT_FILENO))
+    f = NULL;
+  else if (protect_err && !protect_fd (STDERR_FILENO))
+    f = NULL;
+  else
+    f = freopen (name, mode, f);
+  saved_errno = errno;
+  if (protect_err)
+    close (STDERR_FILENO);
+  if (protect_out)
+    close (STDOUT_FILENO);
+  if (protect_in)
+    close (STDIN_FILENO);
+  if (!f)
+    errno = saved_errno;
+  return f;
+}
diff --git a/lib/stdio--.h b/lib/stdio--.h
index ed90dda..d10dd72 100644
--- a/lib/stdio--.h
+++ b/lib/stdio--.h
@@ -25,6 +25,11 @@
 # define fopen fopen_safer
 #endif

+#if GNULIB_FREOPEN_SAFER
+# undef freopen
+# define freopen freopen_safer
+#endif
+
 #if GNULIB_TMPFILE_SAFER
 # undef tmpfile
 # define tmpfile tmpfile_safer
diff --git a/lib/stdio-safer.h b/lib/stdio-safer.h
index dde22f1..cecd7bb 100644
--- a/lib/stdio-safer.h
+++ b/lib/stdio-safer.h
@@ -19,6 +19,18 @@

 #include <stdio.h>

+#if GNULIB_FOPEN_SAFER
 FILE *fopen_safer (char const *, char const *);
+#endif
+
+#if GNULIB_FREOPEN_SAFER
+FILE *freopen_safer (char const *, char const *, FILE *);
+#endif
+
+#if GNULIB_POPEN_SAFER
 FILE *popen_safer (char const *, char const *);
+#endif
+
+#if GNULIB_TMPFILE_SAFER
 FILE *tmpfile_safer (void);
+#endif
diff --git a/m4/stdio-safer.m4 b/m4/stdio-safer.m4
index 9f9d7cc..e3daa21 100644
--- a/m4/stdio-safer.m4
+++ b/m4/stdio-safer.m4
@@ -1,4 +1,4 @@
-#serial 11
+#serial 12
 dnl Copyright (C) 2002, 2005-2007, 2009 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -9,6 +9,11 @@ AC_DEFUN([gl_FOPEN_SAFER],
   AC_LIBOBJ([fopen-safer])
 ])

+AC_DEFUN([gl_FREOPEN_SAFER],
+[
+  AC_LIBOBJ([freopen-safer])
+])
+
 AC_DEFUN([gl_POPEN_SAFER],
 [
   AC_LIBOBJ([popen-safer])
diff --git a/modules/freopen-safer b/modules/freopen-safer
new file mode 100644
index 0000000..91c67b6
--- /dev/null
+++ b/modules/freopen-safer
@@ -0,0 +1,29 @@
+Description:
+freopen function that avoids clobbering std{in,out,err}.
+
+Files:
+lib/stdio--.h
+lib/stdio-safer.h
+lib/freopen-safer.c
+m4/stdio-safer.m4
+
+Depends-on:
+dup2
+freopen
+open
+stdbool
+
+configure.ac:
+gl_FREOPEN_SAFER
+gl_MODULE_INDICATOR([freopen-safer])
+
+Makefile.am:
+
+Include:
+"stdio-safer.h"
+
+License:
+GPL
+
+Maintainer:
+Eric Blake
diff --git a/modules/freopen-safer-tests b/modules/freopen-safer-tests
new file mode 100644
index 0000000..9511880
--- /dev/null
+++ b/modules/freopen-safer-tests
@@ -0,0 +1,10 @@
+Files:
+tests/test-freopen-safer.c
+
+Depends-on:
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-freopen-safer
+check_PROGRAMS += test-freopen-safer
diff --git a/tests/test-freopen-safer.c b/tests/test-freopen-safer.c
new file mode 100644
index 0000000..d1bbe8d
--- /dev/null
+++ b/tests/test-freopen-safer.c
@@ -0,0 +1,107 @@
+/* Test of reopening a stream.
+   Copyright (C) 2009 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/>.  */
+
+/* Written by Eric Blake <address@hidden>, 2009.  */
+
+#include <config.h>
+
+/* Specification.  */
+#include "stdio--.h"
+
+/* Helpers.  */
+#include <stdlib.h>
+#include <unistd.h>
+
+/* This test intentionally closes stderr.  So, we arrange to have fd 10
+   (outside the range of interesting fd's during the test) set up to
+   duplicate the original stderr.  */
+
+#define BACKUP_STDERR_FILENO 10
+static FILE *myerr;
+
+#define ASSERT(expr) \
+  do                                                                         \
+    {                                                                        \
+      if (!(expr))                                                           \
+        {                                                                    \
+          fprintf (myerr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
+          fflush (myerr);                                                    \
+          abort ();                                                          \
+        }                                                                    \
+    }                                                                        \
+  while (0)
+
+int
+main (int argc, char **argv)
+{
+  FILE *fp;
+
+  /* We close fd 2 later, so save it in fd 10.  */
+  if (dup2 (STDERR_FILENO, BACKUP_STDERR_FILENO) != BACKUP_STDERR_FILENO
+      || (myerr = fdopen (BACKUP_STDERR_FILENO, "w")) == NULL)
+    return 2;
+
+  {
+    FILE *tmp;
+    ASSERT (tmp = fopen ("/dev/null", "r"));
+    ASSERT (STDERR_FILENO < fileno (tmp));
+    ASSERT (fp = fopen ("/dev/null", "w"));
+    ASSERT (fileno (tmp) < fileno (fp));
+    ASSERT (fclose (tmp) == 0);
+  }
+
+  /* Gap in fds.  */
+  ASSERT (freopen ("/dev/null", "r+", fp) == fp);
+  ASSERT (STDERR_FILENO < fileno (fp));
+
+  ASSERT (freopen ("/dev/null", "r", stdin) == stdin);
+  ASSERT (STDIN_FILENO == fileno (stdin));
+
+  ASSERT (freopen ("/dev/null", "w", stdout) == stdout);
+  ASSERT (STDOUT_FILENO == fileno (stdout));
+
+  ASSERT (freopen ("/dev/null", "w", stderr) == stderr);
+  ASSERT (STDERR_FILENO == fileno (stderr));
+
+  /* fd 0 closed.  */
+  ASSERT (close (STDIN_FILENO) == 0);
+
+  ASSERT (freopen ("/dev/null", "w", stdout) == stdout);
+  ASSERT (STDOUT_FILENO == fileno (stdout));
+
+  ASSERT (freopen ("/dev/null", "w", stderr) == stderr);
+  ASSERT (STDERR_FILENO == fileno (stderr));
+
+  ASSERT (freopen ("/dev/null", "a", fp) == fp);
+  ASSERT (STDERR_FILENO < fileno (fp));
+
+  /* fd 1 closed.  */
+  ASSERT (close (STDOUT_FILENO) == 0);
+
+  ASSERT (freopen ("/dev/null", "w", stderr) == stderr);
+  ASSERT (STDERR_FILENO == fileno (stderr));
+
+  ASSERT (freopen ("/dev/null", "a+", fp) == fp);
+  ASSERT (STDERR_FILENO < fileno (fp));
+
+  /* fd 2 closed.  */
+  ASSERT (close (STDERR_FILENO) == 0);
+
+  ASSERT (freopen ("/dev/null", "w+", fp) == fp);
+  ASSERT (STDERR_FILENO < fileno (fp));
+
+  return 0;
+}
-- 
1.6.4.2