Re: 4 test failures on freebsd8-rc2

[Eric Blake]

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

According to Eric Blake on 11/10/2009 6:51 AM:
>> At least a couple more to go.  I've confirmed that on FreeBSD:
> 
> Also buggy:
> 
> rm a
> mkfifo b/         => mistakenly creates a

Furthermore, FreeBSD and OpenBSD (and probably NetBSD, but I haven't
checked yet) fail to comply with POSIX in that mknod() fails with EPERM
for non-root even when used to create a pipe.  POSIX is explicit that
mknod must succeed in that case.

Both the BSD /sbin/mknod and coreutils' mknod.c work around this issue by
calling mkfifo under the hood for 'mknod pipe p'.  This was the patch that
I've tested so far for symlink and mkfifoat, but the mkfifo portion still
needs some work due to this latest wrinkle.  I went ahead and pushed the
symlink portion.

I also discovered that readlink is broken on FreeBSD:

$ touch a
$ ln -s a b
$ ln -s b c
$ readlink c/
a              => Oops - read the contents of b.

- --
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/

iEYEARECAAYFAkr6umEACgkQ84KuGfSFAYBmGwCfVFt6qFRBvsh7G8ah/Y5Dp6Ir
vmsAoISWgugY8kyYoAfaiFnUd59XMGGo
=2F10
-----END PGP SIGNATURE-----

>From 73af3ed572cc03ca130cf4cd0b530b0e4b6b77dc Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 10 Nov 2009 07:59:39 -0700
Subject: [PATCH 1/2] symlink: detect FreeBSD bug

symlink(name,"dangling/") mistakenly created a symlink at the
target of "dangling".

* m4/symlink.m4 (gl_FUNC_SYMLINK): Also detect FreeBSD bug with
slash on symlink.
* doc/posix-functions/symlink.texi (symlink): Document the bug.
* tests/test-symlink.h (test_symlink): Enhance test.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                        |    8 ++++++++
 doc/posix-functions/symlink.texi |    2 +-
 m4/symlink.m4                    |   13 ++++++++-----
 tests/test-symlink.h             |   28 +++++++++++++++++++++++-----
 4 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8863561..c8e1374 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,13 @@
 2009-11-10  Eric Blake  <address@hidden>

+       symlink: detect FreeBSD bug
+       * m4/symlink.m4 (gl_FUNC_SYMLINK): Also detect FreeBSD bug with
+       slash on symlink.
+       * doc/posix-functions/symlink.texi (symlink): Document the bug.
+       * tests/test-symlink.h (test_symlink): Enhance test.
+
+2009-11-10  Eric Blake  <address@hidden>
+
        link: detect FreeBSD bug
        * m4/link.m4 (gl_FUNC_LINK): Also detect FreeBSD bug with slash on
        symlink.
diff --git a/doc/posix-functions/symlink.texi b/doc/posix-functions/symlink.texi
index de7c0aa..b3d740f 100644
--- a/doc/posix-functions/symlink.texi
+++ b/doc/posix-functions/symlink.texi
@@ -11,7 +11,7 @@ symlink
 @item
 On some systems, @code{symlink(value,"name/")} mistakenly creates a
 symlink:
-Solaris 9
+FreeBSD 7.2, Solaris 9
 @item
 This function is missing on some platforms; however, the replacement
 always fails with @code{EPERM}:
diff --git a/m4/symlink.m4 b/m4/symlink.m4
index abea045..010d4b8 100644
--- a/m4/symlink.m4
+++ b/m4/symlink.m4
@@ -1,4 +1,4 @@
-# serial 1
+# serial 2
 # See if we need to provide symlink replacement.

 dnl Copyright (C) 2009 Free Software Foundation, Inc.
@@ -13,8 +13,8 @@ AC_DEFUN([gl_FUNC_SYMLINK],
   AC_REQUIRE([gl_UNISTD_H_DEFAULTS])
   AC_CHECK_FUNCS_ONCE([symlink])
   dnl The best we can do on mingw is provide a dummy that always fails, so
-  dnl that compilation can proceed with fewer ifdefs.  On Solaris 9, we
-  dnl want to fix a bug with trailing slash handling.
+  dnl that compilation can proceed with fewer ifdefs.  On Solaris 9 and
+  dnl FreeBSD 7.2, we want to fix a bug with trailing slash handling.
   if test $ac_cv_func_symlink = no; then
     HAVE_SYMLINK=0
     AC_LIBOBJ([symlink])
@@ -24,9 +24,12 @@ AC_DEFUN([gl_FUNC_SYMLINK],
       [AC_RUN_IFELSE(
          [AC_LANG_PROGRAM(
            [[#include <unistd.h>
-]], [[return !symlink ("a", "conftest.link/");]])],
+]], [[if (!symlink ("a", "conftest.link/")) return 1;
+      if (symlink ("conftest.f", "conftest.lnk2")) return 2;
+      if (!symlink ("a", "conftest.lnk2/")) return 3;]])],
          [gl_cv_func_symlink_works=yes], [gl_cv_func_symlink_works=no],
-         [gl_cv_func_symlink_works="guessing no"])])
+         [gl_cv_func_symlink_works="guessing no"])
+      rm -f conftest.f conftest.link conftest.lnk2])
     if test "$gl_cv_func_symlink_works" != yes; then
       REPLACE_SYMLINK=1
       AC_LIBOBJ([symlink])
diff --git a/tests/test-symlink.h b/tests/test-symlink.h
index d009a80..c083c6c 100644
--- a/tests/test-symlink.h
+++ b/tests/test-symlink.h
@@ -35,11 +35,18 @@ test_symlink (int (*func) (char const *, char const *), 
bool print)

   /* Some systems allow the creation of 0-length symlinks as a synonym
      for "."; but most reject it.  */
-  errno = 0;
-  if (func ("", BASE "link2") == -1)
-    ASSERT (errno == ENOENT || errno == EINVAL);
-  else
-    ASSERT (unlink (BASE "link2") == 0);
+  {
+    int status;
+    errno = 0;
+    status = func ("", BASE "link2");
+    if (status == -1)
+      ASSERT (errno == ENOENT || errno == EINVAL);
+    else
+      {
+        ASSERT (status == 0);
+        ASSERT (unlink (BASE "link2") == 0);
+      }
+  }

   /* Sanity checks of failures.  */
   errno = 0;
@@ -69,6 +76,17 @@ test_symlink (int (*func) (char const *, char const *), bool 
print)
   ASSERT (func ("nowhere", BASE "file/") == -1);
   ASSERT (errno == EEXIST || errno == ENOTDIR || errno == ENOENT);

+  /* Trailing slash must always be rejected.  */
+  ASSERT (unlink (BASE "link1") == 0);
+  ASSERT (func (BASE "link2", BASE "link1") == 0);
+  errno = 0;
+  ASSERT (func (BASE "nowhere", BASE "link1/") == -1);
+  ASSERT (errno == EEXIST || errno == ENOTDIR || errno == ENOENT);
+  errno = 0;
+  ASSERT (unlink (BASE "link2") == -1);
+  ASSERT (errno == ENOENT);
+
+  /* Cleanup.  */
   ASSERT (rmdir (BASE "dir") == 0);
   ASSERT (unlink (BASE "file") == 0);
   ASSERT (unlink (BASE "link1") == 0);
-- 
1.6.5.rc1


>From 9550e46ea46d813c1d360db5d818720b98dcf85a Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Tue, 10 Nov 2009 08:58:18 -0700
Subject: [PATCH 2/2] mkfifoat: detect Solaris 9 and FreeBSD bug

On Solaris 9, mkfifoat(fd,"missing/",mode) mistakenly created
"missing".  On FreeBSD 7.2, mkfifoat(fd,"dangling/",mode)
mistakenly created a fifo at the target of "dangling".

* m4/mkfifoat.m4 (gl_FUNC_MKFIFOAT): Detect bugs with trailing
slash handling.
* lib/mkfifoat.c (mkfifoat): Work around the bug.
* modules/mkfifoat (Depends-on): Add lstat.
* doc/posix-functions/mkfifo.texi (mkfifo): Document the bug.
* doc/posix-functions/mknod.texi (mknod): Likewise.
* tests/test-mkfifoat.c (main): Enhance test.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                       |   11 +++++++
 doc/posix-functions/mkfifo.texi |    5 ++-
 doc/posix-functions/mknod.texi  |    5 ++-
 lib/mkfifoat.c                  |   59 +++++++++++++++++++++++++++++++++++++-
 m4/mkfifoat.m4                  |   28 ++++++++++++++++++-
 modules/mkfifoat                |    1 +
 tests/test-mkfifoat.c           |   57 +++++++++++++++++++++++++------------
 7 files changed, 143 insertions(+), 23 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c8e1374..08d6bcf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2009-11-10  Eric Blake  <address@hidden>

+       mkfifoat: detect Solaris 9 and FreeBSD bug
+       * m4/mkfifoat.m4 (gl_FUNC_MKFIFOAT): Detect bugs with trailing
+       slash handling.
+       * lib/mkfifoat.c (mkfifoat): Work around the bug.
+       * modules/mkfifoat (Depends-on): Add lstat.
+       * doc/posix-functions/mkfifo.texi (mkfifo): Document the bug.
+       * doc/posix-functions/mknod.texi (mknod): Likewise.
+       * tests/test-mkfifoat.c (main): Enhance test.
+
+2009-11-10  Eric Blake  <address@hidden>
+
        symlink: detect FreeBSD bug
        * m4/symlink.m4 (gl_FUNC_SYMLINK): Also detect FreeBSD bug with
        slash on symlink.
diff --git a/doc/posix-functions/mkfifo.texi b/doc/posix-functions/mkfifo.texi
index 926f57d..c1792be 100644
--- a/doc/posix-functions/mkfifo.texi
+++ b/doc/posix-functions/mkfifo.texi
@@ -10,9 +10,12 @@ mkfifo
 @itemize
 @end itemize

-Portability problems not fixed by Gnulib:
+Portability problems not fixed by Gnulib (but see the mkfifoat module):
 @itemize
 @item
+This function mishandles trailing slash on some platforms:
+FreeBSD 7.2, Solaris 9.
address@hidden
 This function is missing on some platforms:
 mingw.
 @end itemize
diff --git a/doc/posix-functions/mknod.texi b/doc/posix-functions/mknod.texi
index 2853416..694acac 100644
--- a/doc/posix-functions/mknod.texi
+++ b/doc/posix-functions/mknod.texi
@@ -10,9 +10,12 @@ mknod
 @itemize
 @end itemize

-Portability problems not fixed by Gnulib:
+Portability problems not fixed by Gnulib (but see the mkfifoat module):
 @itemize
 @item
+This function mishandles trailing slash on some platforms:
+FreeBSD 7.2, Solaris 9.
address@hidden
 This function is missing on some platforms:
 mingw.
 @end itemize
diff --git a/lib/mkfifoat.c b/lib/mkfifoat.c
index 29fc070..89376f9 100644
--- a/lib/mkfifoat.c
+++ b/lib/mkfifoat.c
@@ -43,7 +43,7 @@

 int
 mkfifoat (int fd _UNUSED_PARAMETER_, char const *path _UNUSED_PARAMETER_,
-         mode_t mode _UNUSED_PARAMETER_)
+          mode_t mode _UNUSED_PARAMETER_)
 {
   errno = ENOSYS;
   return -1;
@@ -51,7 +51,7 @@ mkfifoat (int fd _UNUSED_PARAMETER_, char const *path 
_UNUSED_PARAMETER_,

 int
 mknodat (int fd _UNUSED_PARAMETER_, char const *path _UNUSED_PARAMETER_,
-        mode_t mode _UNUSED_PARAMETER_, dev_t dev _UNUSED_PARAMETER_)
+         mode_t mode _UNUSED_PARAMETER_, dev_t dev _UNUSED_PARAMETER_)
 {
   errno = ENOSYS;
   return -1;
@@ -59,6 +59,61 @@ mknodat (int fd _UNUSED_PARAMETER_, char const *path 
_UNUSED_PARAMETER_,

 #else /* HAVE_MKFIFO */

+# if MKFIFO_TRAILING_SLASH_BUG
+#  include <errno.h>
+#  include "openat.h"
+
+/* If mkfifo and mknod don't reject trailing slash, then we wrap
+   mkfifoat and mknodat to detect that situation.  */
+
+static int local_mkfifoat (int, char const *, mode_t);
+static int local_mknodat (int, char const *, mode_t, dev_t);
+
+/* Create a fifo NAME relative to FD with permissions in MODE, and
+   work around trailing slash bugs.  */
+int
+mkfifoat (int fd, char const *name, mode_t mode)
+{
+  size_t len = strlen (name);
+  if (len && name[len - 1] == '/')
+    {
+      struct stat st;
+      if (lstatat (fd, name, &st) == 0)
+        errno = EEXIST;
+      return -1;
+    }
+  return local_mkfifoat (fd, name, mode);
+}
+
+/* Create an inode entry NAME relative to FD with permissions in MODE
+   and device type DEV.  If used portably (that is, if MODE specifies
+   a fifo and DEV is 0), work around trailing slash bugs.  */
+int
+mknodat (int fd, char const *name, mode_t mode, dev_t dev)
+{
+  /* Only reject trailing slash when mknodat is being used to create a
+     FIFO; all other uses are implementation-defined, and some
+     implementations might allow the superuser to create a directory
+     using mknod and a trailing slash.  */
+  if (!dev && S_ISFIFO (mode))
+    {
+      size_t len = strlen (name);
+      if (len && name[len - 1] == '/')
+        {
+          struct stat st;
+          if (lstatat (fd, name, &st) == 0)
+            errno = EEXIST;
+          return -1;
+        }
+    }
+  return local_mknodat (fd, name, mode, dev);
+}
+
+# define mkfifoat local_mkfifoat
+# define mknodat local_mknodat
+
+# endif /* MKFIFO_TRAILING_SLASH_BUG */
+
 /* Create a named fifo FILE relative to directory FD, with access
    permissions in MODE.  If possible, do it without changing the
    working directory.  Otherwise, resort to using save_cwd/fchdir,
diff --git a/m4/mkfifoat.m4 b/m4/mkfifoat.m4
index 99e2336..5d28b0b 100644
--- a/m4/mkfifoat.m4
+++ b/m4/mkfifoat.m4
@@ -1,4 +1,4 @@
-# serial 1
+# serial 2
 # See if we need to provide mkfifoat/mknodat replacement.

 dnl Copyright (C) 2009 Free Software Foundation, Inc.
@@ -20,4 +20,30 @@ AC_DEFUN([gl_FUNC_MKFIFOAT],
     HAVE_MKNODAT=0
     AC_LIBOBJ([mkfifoat])
   fi
+  if test $ac_cv_func_mkfifo = yes; then
+    dnl Check for Solaris 9 and FreeBSD bug with trailing slash.
+    dnl No known system has both mkfifoat and trailing slash bug.
+    AC_CHECK_FUNCS_ONCE([lstat])
+    AC_CACHE_CHECK([whether mkfifo rejects trailing slashes],
+      [gl_cv_func_mkfifo_works],
+      [# Assume that if we have lstat, we can also check symlinks.
+       if test $ac_cv_func_lstat = yes; then
+         ln -s conftest.tmp conftest.lnk
+       fi
+       AC_RUN_IFELSE(
+         [AC_LANG_PROGRAM(
+           [[#include <sys/stat.h>
+]], [[if (!mkfifo ("conftest.tmp/", 0600)) return 1;
+#if HAVE_LSTAT
+      if (!mkfifo ("conftest.lnk/", 0600)) return 2;
+#endif
+           ]])],
+         [gl_cv_func_mkfifo_works=yes], [gl_cv_func_mkfifo_works=no],
+         [gl_cv_func_mkfifo_works="guessing no"])
+       rm -f conftest.tmp conftest.lnk])
+    if test "$gl_cv_func_mkfifo_works" != yes; then
+      AC_DEFINE([MKFIFO_TRAILING_SLASH_BUG], [1], [Define to 1 if mkfifo
+        and mknod do not reject trailing slash])
+    fi
+  fi
 ])
diff --git a/modules/mkfifoat b/modules/mkfifoat
index db2f0da..10fdecf 100644
--- a/modules/mkfifoat
+++ b/modules/mkfifoat
@@ -8,6 +8,7 @@ m4/mkfifoat.m4
 Depends-on:
 extensions
 fcntl-h
+lstat
 openat
 sys_stat

diff --git a/tests/test-mkfifoat.c b/tests/test-mkfifoat.c
index 2992ba2..ea26f17 100644
--- a/tests/test-mkfifoat.c
+++ b/tests/test-mkfifoat.c
@@ -31,14 +31,16 @@
   do                                                                         \
     {                                                                        \
       if (!(expr))                                                           \
-       {                                                                    \
-         fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
-         fflush (stderr);                                                   \
-         abort ();                                                          \
-       }                                                                    \
+        {                                                                    \
+          fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
+          fflush (stderr);                                                   \
+          abort ();                                                          \
+        }                                                                    \
     }                                                                        \
   while (0)

+#define BASE "test-mkfifoat.t"
+
 typedef int (*test_func) (int, char const *, mode_t);

 /* Wrapper to make testing mknodat easier.  */
@@ -54,7 +56,6 @@ main (void)
 {
   int i;
   test_func funcs[2] = { mkfifoat, test_mknodat };
-  const char *fifo = "test-mkfifoat.fifo";

   /* Create handle for future use.  */
   int dfd = openat (AT_FDCWD, ".", O_RDONLY);
@@ -65,8 +66,8 @@ main (void)
   return 77;
 #endif

-  /* Clean up anything from previous incomplete test.  */
-  remove (fifo);
+  /* Remove any leftovers from a previous partial run.  */
+  ASSERT (system ("rm -rf " BASE "*") == 0);

   /* Test both functions.  */
   for (i = 0; i < 2; i++)
@@ -90,35 +91,55 @@ main (void)
       ASSERT (errno == EEXIST || errno == EINVAL);

       /* Create fifo while cwd is '.', then stat it from '..'.  */
-      ASSERT (func (AT_FDCWD, fifo, 0600) == 0);
+      ASSERT (func (AT_FDCWD, BASE "fifo", 0600) == 0);
       errno = 0;
-      ASSERT (func (dfd, fifo, 0600) == -1);
+      ASSERT (func (dfd, BASE "fifo", 0600) == -1);
       ASSERT (errno == EEXIST);
       ASSERT (chdir ("..") == 0);
       errno = 0;
-      ASSERT (fstatat (AT_FDCWD, fifo, &st, 0) == -1);
+      ASSERT (fstatat (AT_FDCWD, BASE "fifo", &st, 0) == -1);
       ASSERT (errno == ENOENT);
       memset (&st, 0, sizeof st);
-      ASSERT (fstatat (dfd, fifo, &st, 0) == 0);
+      ASSERT (fstatat (dfd, BASE "fifo", &st, 0) == 0);
       ASSERT (S_ISFIFO (st.st_mode));
-      ASSERT (unlinkat (dfd, fifo, 0) == 0);
+      ASSERT (unlinkat (dfd, BASE "fifo", 0) == 0);

       /* Create fifo while cwd is '..', then stat it from '.'.  */
-      ASSERT (func (dfd, fifo, 0600) == 0);
+      ASSERT (func (dfd, BASE "fifo", 0600) == 0);
       ASSERT (fchdir (dfd) == 0);
       errno = 0;
-      ASSERT (func (AT_FDCWD, fifo, 0600) == -1);
+      ASSERT (func (AT_FDCWD, BASE "fifo", 0600) == -1);
       ASSERT (errno == EEXIST);
       memset (&st, 0, sizeof st);
-      ASSERT (fstatat (AT_FDCWD, fifo, &st, AT_SYMLINK_NOFOLLOW) == 0);
+      ASSERT (fstatat (AT_FDCWD, BASE "fifo", &st, AT_SYMLINK_NOFOLLOW) == 0);
       ASSERT (S_ISFIFO (st.st_mode));
       memset (&st, 0, sizeof st);
-      ASSERT (fstatat (dfd, fifo, &st, AT_SYMLINK_NOFOLLOW) == 0);
+      ASSERT (fstatat (dfd, BASE "fifo", &st, AT_SYMLINK_NOFOLLOW) == 0);
       ASSERT (S_ISFIFO (st.st_mode));
-      ASSERT (unlink (fifo) == 0);
+      ASSERT (unlink (BASE "fifo") == 0);
     }

   ASSERT (close (dfd) == 0);

+  /* Test trailing slash behavior.  */
+  if (symlink (BASE "fifo", BASE "link"))
+    {
+      fputs ("skipping test: symlinks not supported on this file system\n",
+             stderr);
+      return 77;
+    }
+  for (i = 0; i < 2; i++)
+    {
+      test_func func = funcs[i];
+      errno = 0;
+      ASSERT (func (AT_FDCWD, BASE "fifo/", 0600) == -1);
+      ASSERT (errno == EINVAL || errno == ENOENT || errno == ENOTDIR);
+      errno = 0;
+      ASSERT (func (AT_FDCWD, BASE "link/", 0600) == -1);
+      ASSERT (errno == EINVAL || errno == ENOENT || errno == ENOTDIR
+              || errno == EEXIST);
+    }
+  ASSERT (unlink (BASE "link") == 0);
+
   return 0;
 }
-- 
1.6.5.rc1