[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: mktemp write failure
From: |
Eric Blake |
Subject: |
Re: mktemp write failure |
Date: |
Fri, 6 Nov 2009 18:13:46 +0000 (UTC) |
User-agent: |
Loom/3.14 (http://gmane.org/) |
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
- Re: temp file suffixes: mktemp DWIM, (continued)
- Re: temp file suffixes: mktemp DWIM, Jim Meyering, 2009/11/03
- Re: temp file suffixes: mktemp DWIM, Pádraig Brady, 2009/11/03
- Re: temp file suffixes: mktemp DWIM, Eric Blake, 2009/11/04
- Re: temp file suffixes: mktemp DWIM, Eric Blake, 2009/11/04
- Re: temp file suffixes: mktemp DWIM, Jim Meyering, 2009/11/04
- mktemp write failure (was: temp file suffixes: mktemp DWIM), Eric Blake, 2009/11/05
- Re: mktemp write failure, Eric Blake, 2009/11/05
- Re: mktemp write failure, Eric Blake, 2009/11/05
- Re: mktemp write failure, Jim Meyering, 2009/11/06
- Re: mktemp write failure,
Eric Blake <=
- Re: mktemp write failure, Jim Meyering, 2009/11/06
- Re: mktemp write failure, Eric Blake, 2009/11/06
- Re: mktemp write failure, Jim Meyering, 2009/11/05
Re: temp file suffixes: mktemp DWIM, Eric Blake, 2009/11/09