[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: need opendir_safer, dirent--.h
|
From: |
Eric Blake |
|
Subject: |
Re: need opendir_safer, dirent--.h |
|
Date: |
Sat, 19 Sep 2009 08:17:04 -0600 |
|
User-agent: |
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.23) Gecko/20090812 Thunderbird/2.0.0.23 Mnenhy/0.7.6.666 |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
According to Eric Blake on 9/2/2009 1:00 PM:
> And here's openat-safer, plus a rework of making fts safer.
> Subject: [PATCH 2/2] backupfile, chdir-long, fts, savedir: make safer
>
> * lib/backupfile.c (includes): Use "dirent--.h", since
> numbered_backup can write to stderr during readdir.
> * lib/savedir.c (includes): Likewise.
> * lib/chdir-long.c (includes): Use "fcntl--.h", since openat
> emulation can write to stderr on failure.
Since openat requires chdir-long, and chdir-long requires openat-safer,
but openat-safer implies that rpl_openat uses open_safer, there is _no
way_ to use the openat replacement to get fd 0. This seems a bit
restrictive; I think that openat-safer should be a conscious choice, not
dragged in by default for just openat (however, I do admit that the use of
openat without *-safer will probably be rare). It is also inconsistent -
platforms with native openat behave differently than those with the
replacement, and the difference can mask subtle bugs with stdin.
To fix it, I'm committing this. Note that the only reason that chdir-long
worried about openat-safer was that if the openat emulation could succeeds
in opening fd 2, but then fail to restore the current working directory,
it would then clobber fd 2 during openat_restore_fail. So, fix this
obscure corner case by closing a just-created fd 2 before dying.
Meanwhile, save_cwd _needs_ to be safe, but can do so manually via
dup_safer rather than having a blind dependency on fcntl-safer.
I've tested that, with this patch, './gnulib-tool --with-tests --test
openat' no longer drags in fcntl-safer, but './gnulib-tool --with-tests
- --test openat-safer' still passes the new test-openat.
There's also a followup to make the use of at-func.c easier (client files
should not need to know which include files were needed by at-func.c), for
some upcoming patches for Solaris 9 fstatat and unlinkat.
- --
Don't work too hard, make some time for fun as well!
Eric Blake address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAkq05+AACgkQ84KuGfSFAYC3UwCfQJ9Lmd1Oi9Z55GkaUD44AsYa
lZ8AoI8EU/UqJyQErkuRi3ofY0NxcdBv
=stXI
-----END PGP SIGNATURE-----
>From 39ed9bd6ad79e753116be0242400d4586c914bf7 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 19 Sep 2009 07:12:15 -0600
Subject: [PATCH 1/2] openat: allow return of fd 0
Partially reverts patch fc33350 from 2009-09-02.
* modules/chdir-long (Depends-on): Relax openat-safer to openat.
* modules/save-cwd (Depends-on): Replace fcntl-safer with
unistd-safer.
* lib/chdir-long.c (includes): Replace "fcntl--.h" with
<fcntl.h>; this module does not leak fds.
* lib/openat.c (includes): Do not use "fcntl_safer"; plain openat
must be allowed to return 0, leaving openat_safer to add the
safety.
(openat_permissive): Avoid writing to just-opened fd 2 if
restoring the current directory fails.
* lib/openat-die.c (openat_restore_fail): Add comment.
* lib/save-cwd.c (includes): Make "fcntl--.h" conditional.
(save_cwd): Guarantee safe fd, but without use of open_safer.
* tests/test-openat.c: New test.
* modules/openat-tests (Files, Makefile.am): Distribute and build
new file.
Signed-off-by: Eric Blake <address@hidden>
---
ChangeLog | 18 +++++++++++++++
lib/chdir-long.c | 12 ++++++---
lib/openat-die.c | 5 ++++
lib/openat.c | 15 +++++------
lib/save-cwd.c | 14 +++++++++--
modules/chdir-long | 2 +-
modules/openat-tests | 6 +++-
modules/save-cwd | 4 +-
tests/test-openat.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
9 files changed, 116 insertions(+), 20 deletions(-)
create mode 100644 tests/test-openat.c
diff --git a/ChangeLog b/ChangeLog
index a514b34..3e8e24b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,23 @@
2009-09-19 Eric Blake <address@hidden>
+ openat: allow return of fd 0
+ * modules/chdir-long (Depends-on): Relax openat-safer to openat.
+ * modules/save-cwd (Depends-on): Replace fcntl-safer with
+ unistd-safer.
+ * lib/chdir-long.c (includes): Replace "fcntl--.h" with
+ <fcntl.h>; this module does not leak fds.
+ * lib/openat.c (includes): Do not use "fcntl_safer"; plain openat
+ must be allowed to return 0, leaving openat_safer to add the
+ safety.
+ (openat_permissive): Avoid writing to just-opened fd 2 if
+ restoring the current directory fails.
+ * lib/openat-die.c (openat_restore_fail): Add comment.
+ * lib/save-cwd.c (includes): Make "fcntl--.h" conditional.
+ (save_cwd): Guarantee safe fd, but without use of open_safer.
+ * tests/test-openat.c: New test.
+ * modules/openat-tests (Files, Makefile.am): Distribute and build
+ new file.
+
relocatable-prog-wrapper: fix build
* modules/relocatable-prog-wrapper (Files): Update name of
canonicalize m4 file, broken on 2009-09-17.
diff --git a/lib/chdir-long.c b/lib/chdir-long.c
index ba47d59..afe018d 100644
--- a/lib/chdir-long.c
+++ b/lib/chdir-long.c
@@ -20,19 +20,23 @@
#include "chdir-long.h"
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>
-#include <errno.h>
#include <stdio.h>
-#include <assert.h>
-
-#include "fcntl--.h"
#ifndef PATH_MAX
# error "compile this file only if your system defines PATH_MAX"
#endif
+/* The results of openat() in this file are not leaked to any
+ single-threaded code that could use stdio.
+ FIXME - if the kernel ever adds support for multi-thread safety for
+ avoiding standard fds, then we should use openat_safer. */
+
struct cd_buf
{
int fd;
diff --git a/lib/openat-die.c b/lib/openat-die.c
index 677f3e0..a507eac 100644
--- a/lib/openat-die.c
+++ b/lib/openat-die.c
@@ -40,6 +40,11 @@ openat_save_fail (int errnum)
abort ();
}
+
+/* Exit with an error about failure to restore the working directory
+ during an openat emulation. The caller must ensure that fd 2 is
+ not a just-opened fd, even when openat_safer is not in use. */
+
void
openat_restore_fail (int errnum)
{
diff --git a/lib/openat.c b/lib/openat.c
index e1471b8..d22d830 100644
--- a/lib/openat.c
+++ b/lib/openat.c
@@ -28,13 +28,6 @@
#include "openat-priv.h"
#include "save-cwd.h"
-/* We can't use "fcntl--.h", so that openat_safer does not interfere. */
-#if GNULIB_FCNTL_SAFER
-# include "fcntl-safer.h"
-# undef open
-# define open open_safer
-#endif
-
/* Replacement for Solaris' openat function.
<http://www.google.com/search?q=openat+site:docs.sun.com>
First, try to simulate it via open ("/proc/self/fd/FD/FILE").
@@ -124,7 +117,13 @@ openat_permissive (int fd, char const *file, int flags,
mode_t mode,
if (save_ok && restore_cwd (&saved_cwd) != 0)
{
if (! cwd_errno)
- openat_restore_fail (errno);
+ {
+ /* Don't write a message to just-created fd 2. */
+ saved_errno = errno;
+ if (err == STDERR_FILENO)
+ close (err);
+ openat_restore_fail (saved_errno);
+ }
*cwd_errno = errno;
}
}
diff --git a/lib/save-cwd.c b/lib/save-cwd.c
index e158e8b..7c2eb6d 100644
--- a/lib/save-cwd.c
+++ b/lib/save-cwd.c
@@ -1,6 +1,6 @@
/* save-cwd.c -- Save and restore current working directory.
- Copyright (C) 1995, 1997, 1998, 2003, 2004, 2005, 2006 Free
+ Copyright (C) 1995, 1997, 1998, 2003, 2004, 2005, 2006, 2009 Free
Software Foundation, Inc.
This program is free software: you can redistribute it and/or modify
@@ -23,15 +23,21 @@
#include "save-cwd.h"
#include <errno.h>
+#include <fcntl.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
-#include <unistd.h>
#include "chdir-long.h"
-#include "fcntl--.h"
+#include "unistd--.h"
#include "xgetcwd.h"
+#if GNULIB_FCNTL_SAFER
+# include "fcntl--.h"
+#else
+# define GNULIB_FCNTL_SAFER 0
+#endif
+
/* On systems without the fchdir function (WOE), pretend that open
always returns -1 so that save_cwd resorts to using xgetcwd.
Since chdir_long requires fchdir, use chdir instead. */
@@ -70,6 +76,8 @@ save_cwd (struct saved_cwd *cwd)
cwd->name = NULL;
cwd->desc = open (".", O_RDONLY);
+ if (!GNULIB_FCNTL_SAFER)
+ cwd->desc = fd_safer (cwd->desc);
if (cwd->desc < 0)
{
cwd->name = xgetcwd ();
diff --git a/modules/chdir-long b/modules/chdir-long
index cdcb9eb..4025b45 100644
--- a/modules/chdir-long
+++ b/modules/chdir-long
@@ -10,7 +10,7 @@ Depends-on:
atexit
fchdir
fcntl-h
-openat-safer
+openat
memchr
mempcpy
memrchr
diff --git a/modules/openat-tests b/modules/openat-tests
index a82a4f3..54d0c61 100644
--- a/modules/openat-tests
+++ b/modules/openat-tests
@@ -1,5 +1,6 @@
Files:
tests/test-rmdir.h
+tests/test-openat.c
tests/test-unlinkat.c
Depends-on:
@@ -8,6 +9,7 @@ configure.ac:
AC_CHECK_FUNCS_ONCE([symlink])
Makefile.am:
-TESTS += test-unlinkat
-check_PROGRAMS += test-unlinkat
+TESTS += test-openat test-unlinkat
+check_PROGRAMS += test-openat test-unlinkat
+test_openat_LDADD = $(LDADD) @LIBINTL@
test_unlinkat_LDADD = $(LDADD) @LIBINTL@
diff --git a/modules/save-cwd b/modules/save-cwd
index 18c43b9..46a1276 100644
--- a/modules/save-cwd
+++ b/modules/save-cwd
@@ -8,9 +8,9 @@ m4/save-cwd.m4
Depends-on:
chdir-long
-fcntl-safer
-xgetcwd
stdbool
+unistd-safer
+xgetcwd
configure.ac:
gl_SAVE_CWD
diff --git a/tests/test-openat.c b/tests/test-openat.c
new file mode 100644
index 0000000..8fa8f83
--- /dev/null
+++ b/tests/test-openat.c
@@ -0,0 +1,60 @@
+/* Test that openat works.
+ 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>
+
+#include <fcntl.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+#define ASSERT(expr) \
+ do \
+ { \
+ if (!(expr)) \
+ { \
+ fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
+ fflush (stderr); \
+ abort (); \
+ } \
+ } \
+ while (0)
+
+int
+main ()
+{
+ /* FIXME - add more tests. For example, share /dev/null and
+ trailing slash tests with test-open, and do more checks for
+ proper fd handling. */
+
+ /* Check that even when *-safer modules are in use, plain openat can
+ land in fd 0. Do this test last, since it is destructive to
+ stdin. */
+ ASSERT (close (STDIN_FILENO) == 0);
+ ASSERT (openat (AT_FDCWD, ".", O_RDONLY) == STDIN_FILENO);
+ {
+ int dfd = open (".", O_RDONLY);
+ ASSERT (STDIN_FILENO < dfd);
+ ASSERT (chdir ("..") == 0);
+ ASSERT (close (STDIN_FILENO) == 0);
+ ASSERT (openat (dfd, ".", O_RDONLY) == STDIN_FILENO);
+ ASSERT (close (dfd) == 0);
+ }
+ return 0;
+}
--
1.6.5.rc1
>From 8ea93ae4889655a9469363d6fbe739b902984c71 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 19 Sep 2009 08:03:45 -0600
Subject: [PATCH 2/2] openat: simplify use of at-func.c
* lib/at-func.c (includes): Include prerequisites here, to
simplify requirements on client files.
* lib/openat-priv.h: Add double-inclusion guard.
* lib/faccessat.c (includes): Simplify.
* lib/fchmodat.c (includes): Likewise.
* lib/fchownat.c (includes): Likewise.
* lib/mkdirat.c (includes): Likewise.
* lib/mkfifoat.c (includes): Likewise.
* lib/symlinkat.c (includes): Likewise.
Signed-off-by: Eric Blake <address@hidden>
---
ChangeLog | 11 +++++++++++
lib/at-func.c | 5 +++++
lib/faccessat.c | 5 -----
lib/fchmodat.c | 5 +----
lib/fchownat.c | 5 -----
lib/mkdirat.c | 5 -----
lib/mkfifoat.c | 5 -----
lib/openat-priv.h | 5 +++++
lib/symlinkat.c | 5 -----
9 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 3e8e24b..6b17480 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
2009-09-19 Eric Blake <address@hidden>
+ openat: simplify use of at-func.c
+ * lib/at-func.c (includes): Include prerequisites here, to
+ simplify requirements on client files.
+ * lib/openat-priv.h: Add double-inclusion guard.
+ * lib/faccessat.c (includes): Simplify.
+ * lib/fchmodat.c (includes): Likewise.
+ * lib/fchownat.c (includes): Likewise.
+ * lib/mkdirat.c (includes): Likewise.
+ * lib/mkfifoat.c (includes): Likewise.
+ * lib/symlinkat.c (includes): Likewise.
+
openat: allow return of fd 0
* modules/chdir-long (Depends-on): Relax openat-safer to openat.
* modules/save-cwd (Depends-on): Replace fcntl-safer with
diff --git a/lib/at-func.c b/lib/at-func.c
index 3188770..75c80d3 100644
--- a/lib/at-func.c
+++ b/lib/at-func.c
@@ -16,6 +16,11 @@
/* written by Jim Meyering */
+#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
+#include "openat.h"
+#include "openat-priv.h"
+#include "save-cwd.h"
+
#ifdef AT_FUNC_USE_F1_COND
# define CALL_FUNC(F) \
(flag == AT_FUNC_USE_F1_COND \
diff --git a/lib/faccessat.c b/lib/faccessat.c
index 42479c7..de2dc2e 100644
--- a/lib/faccessat.c
+++ b/lib/faccessat.c
@@ -20,11 +20,6 @@
#include <unistd.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
-
#ifndef HAVE_ACCESS
/* Mingw lacks access, but it also lacks real vs. effective ids, so
the gnulib euidaccess module is good enough. */
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index 53cafe0..55ae618 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -20,10 +20,7 @@
#include <sys/stat.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
+#include <errno.h>
#ifndef HAVE_LCHMOD
/* Use a different name, to avoid conflicting with any
diff --git a/lib/fchownat.c b/lib/fchownat.c
index 6da3537..09b4aa8 100644
--- a/lib/fchownat.c
+++ b/lib/fchownat.c
@@ -25,11 +25,6 @@
#include <unistd.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
-
/* Replacement for Solaris' function by the same name.
Invoke chown or lchown on file, FILE, using OWNER and GROUP, in the
directory open on descriptor FD. If FLAG is AT_SYMLINK_NOFOLLOW, then
diff --git a/lib/mkdirat.c b/lib/mkdirat.c
index 33ece9c..adb11f0 100644
--- a/lib/mkdirat.c
+++ b/lib/mkdirat.c
@@ -20,11 +20,6 @@
#include <unistd.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
-
/* Solaris 10 has no function like this.
Create a subdirectory, FILE, with mode MODE, in the directory
open on descriptor FD. If possible, do it without changing the
diff --git a/lib/mkfifoat.c b/lib/mkfifoat.c
index a230b00..29fc070 100644
--- a/lib/mkfifoat.c
+++ b/lib/mkfifoat.c
@@ -20,11 +20,6 @@
#include <sys/stat.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
-
#ifndef HAVE_MKFIFO
# define HAVE_MKFIFO 0
#endif
diff --git a/lib/openat-priv.h b/lib/openat-priv.h
index 2280416..fa286b5 100644
--- a/lib/openat-priv.h
+++ b/lib/openat-priv.h
@@ -17,6 +17,9 @@
/* written by Jim Meyering */
+#ifndef _GL_HEADER_OPENAT_PRIV
+#define _GL_HEADER_OPENAT_PRIV
+
#include <errno.h>
#include <stdlib.h>
@@ -32,3 +35,5 @@ char *openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd,
char const *file);
|| (Errno) == EPERM || (Errno) == EACCES \
|| (Errno) == ENOSYS /* Solaris 8 */ \
|| (Errno) == EOPNOTSUPP /* FreeBSD */)
+
+#endif /* _GL_HEADER_OPENAT_PRIV */
diff --git a/lib/symlinkat.c b/lib/symlinkat.c
index 6fe752f..fd129ea 100644
--- a/lib/symlinkat.c
+++ b/lib/symlinkat.c
@@ -20,11 +20,6 @@
#include <unistd.h>
-#include "dirname.h" /* solely for definition of IS_ABSOLUTE_FILE_NAME */
-#include "openat.h"
-#include "openat-priv.h"
-#include "save-cwd.h"
-
#if !HAVE_SYMLINK
/* Mingw lacks symlink, so this wrapper is trivial. */
--
1.6.5.rc1