Re: mktemp write failure

[Eric Blake]

Jim Meyering <jim <at> meyering.net> writes:

> I'm pretty sure I've already audited each of those, and assured myself
> that they were not vulnerable to this sort of trouble -- but that was back
> when we added other *safer functions.  Thus, if they currently solve no
> problem, I'm a little reluctant to impose the wrappers.  On the other,
> I haven't measured the cost of going through the wrapper, and we've just
> seen how the lack of the wrapper can cause an admittedly-obscure failure.
> 
> It is probably worth making this change regardless, just because
> the issue is so subtle.
> 
> Have you considered automating (make syntax-check style) the task of
> ensuring that stdio--.h is included at least whenever freopen is used?

Like this?  Rebased against today's changes.  As shown below in patch 3/3, the 
patch requires stdio--.h only for freopen.  But I also tried this regex in 
sc_require_stdio_safer:

          files=$$(grep -l '\b\(f\|fre\|p\)open \?(' $$($(VC_LIST_EXCEPT) \

and found the following culprits (I think they were all due to fopen).

src/base64.c
src/cksum.c
src/cut.c
src/date.c
src/expand.c
src/fmt.c
src/fold.c
src/nl.c
src/od.c
src/paste.c
src/pinky.c
src/sum.c
src/unexpand.c
src/uptime.c
src/wc.c
maint.mk: the above files should use "stdio--.h"

I checked out base64; it looks like it could possibly benefit from using freopen
(,stdin) instead of fopen, but I couldn't come up with any scenario where 
starting life with a closed fd would cause problems (with >&-, input_fh ends up 
on descriptor 1, but since fd 1 is O_RDONLY in that case, attempts to use 
stdout still gave the expected failure).  But that doesn't mean there aren't 
any issues, since I didn't try using things like /proc/self/fd/1 as the file 
name.  Likewise, I did not audit the rest of the above list to see if 
fopen_safer would make an observable difference.

On the other hand, we are not calling fopen frequently, so it is not on the hot 
path, and the time spent in an extra wrapper for safety is probably 
insignificant.  So, should I respin this patch to use the regex that catches 
fopen, and add "stdio--.h" to even more files?  Or maybe even squash patch 2 
and 3 into a single one before pushing?


>From d6f639fa859d6457a1917c50acdd1a139f56c9b9 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Thu, 5 Nov 2009 12:19:45 -0700
Subject: [PATCH 1/3] mktemp: fix bug with -q and closed stdout

If stdin or stdout is closed, then freopen(,stderr) can violate
the premise that STDERR_FILENO==fileno(stderr), which in turn
breaks mktemp -q.

* bootstrap.conf (gnulib_modules): Add freopen-safer.
* src/mktemp.c (includes): Use stdio--.h.
* tests/misc/close-stdout: Enhance test to catch bug.
---
 bootstrap.conf          |    1 +
 src/mktemp.c            |    2 +-
 tests/misc/close-stdout |    6 ++++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 3a26394..d1e80a4 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -92,6 +92,7 @@ gnulib_modules="
   fopen-safer
   fprintftime
   freopen
+  freopen-safer
   fseeko
   fsusage
   fsync
diff --git a/src/mktemp.c b/src/mktemp.c
index 6ce40f1..303b9ce 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -17,7 +17,6 @@
 /* Written by Jim Meyering and Eric Blake.  */

 #include <config.h>
-#include <stdio.h>
 #include <sys/types.h>
 #include <getopt.h>

@@ -27,6 +26,7 @@
 #include "error.h"
 #include "filenamecat.h"
 #include "quote.h"
+#include "stdio--.h"
 #include "tempname.h"

 /* The official name of this program (e.g., no `g' prefix).  */
diff --git a/tests/misc/close-stdout b/tests/misc/close-stdout
index 852c3c8..ae2350d 100755
--- a/tests/misc/close-stdout
+++ b/tests/misc/close-stdout
@@ -52,7 +52,8 @@ if "$p/src/test" -w /dev/stdout >/dev/null &&
   cp --verbose a b >&- 2>/dev/null && fail=1
   rm -Rf tmpfile-?????? || fail=1
   mktemp tmpfile-XXXXXX >&- 2>/dev/null && fail=1
-  test -e tmpfile-?????? && fail=1
+  mktemp tmpfile-XXXXXX -q >&- 2>/dev/null && fail=1
+  case `echo tmpfile-??????` in 'tmpfile-??????') ;; *) fail=1 ;; esac
 fi

 # Likewise for /dev/full, if /dev/full works.
@@ -61,7 +62,8 @@ if test -w /dev/full && test -c /dev/full; then
   cp --verbose a b >/dev/full 2>/dev/null && fail=1
   rm -Rf tmpdir-?????? || fail=1
   mktemp -d tmpdir-XXXXXX >/dev/full 2>/dev/null && fail=1
-  test -e tmpdir-?????? && fail=1
+  mktemp -d -q tmpdir-XXXXXX >/dev/full 2>/dev/null && fail=1
+  case `echo tmpfile-??????` in 'tmpfile-??????') ;; *) fail=1 ;; esac
 fi

 Exit $fail
-- 
1.6.4.2


>From 4ec32705ae8db74a90cd09d03a306ef6099b3691 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Thu, 5 Nov 2009 16:48:09 -0700
Subject: [PATCH 2/3] build: consistently use freopen-safer

cat, head, ptx, shuf, tac, tail, tee, tr, and uniq were affected

* gl/modules/xfreopen (Depends-on): Add freopen-safer.
* gl/lib/xfreopen.c (includes): Use stdio--.h.
* src/ptx.c (includes): Likewise.
* src/shuf.c (includes): Likewise.
* src/uniq.c (includes): Likewise.
---
 gl/lib/xfreopen.c   |    1 +
 gl/modules/xfreopen |    1 +
 src/ptx.c           |    2 +-
 src/shuf.c          |    2 +-
 src/uniq.c          |    2 +-
 5 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gl/lib/xfreopen.c b/gl/lib/xfreopen.c
index 6109169..32e68fa 100644
--- a/gl/lib/xfreopen.c
+++ b/gl/lib/xfreopen.c
@@ -21,6 +21,7 @@
 #include "error.h"
 #include "exitfail.h"
 #include "quote.h"
+#include "stdio--.h"

 #include "gettext.h"
 #define _(msgid) gettext (msgid)
diff --git a/gl/modules/xfreopen b/gl/modules/xfreopen
index 411f80b..ed4ede7 100644
--- a/gl/modules/xfreopen
+++ b/gl/modules/xfreopen
@@ -8,6 +8,7 @@ lib/xfreopen.h
 Depends-on:
 error
 exitfail
+freopen-safer
 quote

 configure.ac:
diff --git a/src/ptx.c b/src/ptx.c
index 4947a0f..701fcb3 100644
--- a/src/ptx.c
+++ b/src/ptx.c
@@ -19,7 +19,6 @@

 #include <config.h>

-#include <stdio.h>
 #include <getopt.h>
 #include <sys/types.h>
 #include "system.h"
@@ -29,6 +28,7 @@
 #include "quote.h"
 #include "quotearg.h"
 #include "regex.h"
+#include "stdio--.h"
 #include "xstrtol.h"

 /* The official name of this program (e.g., no `g' prefix).  */
diff --git a/src/shuf.c b/src/shuf.c
index 0bb11ab..71411a4 100644
--- a/src/shuf.c
+++ b/src/shuf.c
@@ -19,7 +19,6 @@

 #include <config.h>

-#include <stdio.h>
 #include <sys/types.h>
 #include "system.h"

@@ -29,6 +28,7 @@
 #include "quotearg.h"
 #include "randint.h"
 #include "randperm.h"
+#include "stdio--.h"
 #include "xstrtol.h"

 /* The official name of this program (e.g., no `g' prefix).  */
diff --git a/src/uniq.c b/src/uniq.c
index 7509bfc..ac7ecac 100644
--- a/src/uniq.c
+++ b/src/uniq.c
@@ -18,7 +18,6 @@
 
 #include <config.h>

-#include <stdio.h>
 #include <getopt.h>
 #include <sys/types.h>

@@ -29,6 +28,7 @@
 #include "hard-locale.h"
 #include "posixver.h"
 #include "quote.h"
+#include "stdio--.h"
 #include "xmemcoll.h"
 #include "xstrtol.h"
 #include "memcasecmp.h"
-- 
1.6.4.2


>From ce9a312827105ff0af986a3765cca445fe7e7c08 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Fri, 6 Nov 2009 11:08:22 -0700
Subject: [PATCH 3/3] build: more uses of freopen-safer

* cfg.mk (sc_require_stdio_safer): New rule.
* src/dircolors.c (includes): Use stdio--.h.
* src/du.c (includes): Likewise.
* src/tsort.c (includes): Likewise.
---
 cfg.mk          |   12 ++++++++++++
 src/dircolors.c |    2 +-
 src/du.c        |    2 +-
 src/tsort.c     |    2 +-
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 27349d0..03c3fce 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -237,4 +237,16 @@ sc_prohibit_fail_0:
        msg='fail=0 initialization'                                     \
          $(_prohibit_regexp)

+# Ensure that "stdio--.h" is used where appropriate.
+sc_require_stdio_safer:
+       @if $(VC_LIST_EXCEPT) | grep -l '\.[ch]$$' > /dev/null; then    \
+         files=$$(grep -l '\bfreopen \?(' $$($(VC_LIST_EXCEPT)         \
+             | grep '\.[ch]$$'));                                      \
+         test -n "$$files" && grep -LE 'include "stdio--.h"' $$files   \
+             | grep . &&                                               \
+         { echo '$(ME): the above files should use "stdio--.h"'        \
+               1>&2; exit 1; } || :;                                   \
+       else :;                                                         \
+       fi
+
 include $(srcdir)/dist-check.mk
diff --git a/src/dircolors.c b/src/dircolors.c
index f28487e..54139ba 100644
--- a/src/dircolors.c
+++ b/src/dircolors.c
@@ -19,7 +19,6 @@

 #include <sys/types.h>
 #include <getopt.h>
-#include <stdio.h>

 #include "system.h"
 #include "dircolors.h"
@@ -27,6 +26,7 @@
 #include "error.h"
 #include "obstack.h"
 #include "quote.h"
+#include "stdio--.h"
 #include "xstrndup.h"

 /* The official name of this program (e.g., no `g' prefix).  */
diff --git a/src/du.c b/src/du.c
index c33bbb7..93a33cf 100644
--- a/src/du.c
+++ b/src/du.c
@@ -24,7 +24,6 @@
    Rewritten to use nftw, then to use fts by Jim Meyering.  */

 #include <config.h>
-#include <stdio.h>
 #include <getopt.h>
 #include <sys/types.h>
 #include <assert.h>
@@ -40,6 +39,7 @@
 #include "quotearg.h"
 #include "same.h"
 #include "stat-time.h"
+#include "stdio--.h"
 #include "xfts.h"
 #include "xstrtol.h"

diff --git a/src/tsort.c b/src/tsort.c
index 09067f2..cc6807a 100644
--- a/src/tsort.c
+++ b/src/tsort.c
@@ -22,7 +22,6 @@

 #include <config.h>

-#include <stdio.h>
 #include <assert.h>
 #include <getopt.h>
 #include <sys/types.h>
@@ -32,6 +31,7 @@
 #include "error.h"
 #include "quote.h"
 #include "readtokens.h"
+#include "stdio--.h"

 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "tsort"
-- 
1.6.4.2