Re: 4 test failures on freebsd8-rc2

[Eric Blake]

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

According to Eric Blake on 11/7/2009 5:12 PM:
> Here's part of the difference - Solaris 9 mistakenly allows stat("file/")
> and stat("link-to-file/") to succeed, while FreeBSD only allowed the
> latter. Since the m4 test only covered the former, we weren't replacing
> stat on FreeBSD, even though we needed to.

Likewise for open (test-utimens passes once stat and open are fixed).

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

iEYEARECAAYFAkr2Uq8ACgkQ84KuGfSFAYAzLACePMGkOgHql40CGIvYuZjJ0HkX
KgoAoI1Dy60Oh/UBYZciy7BzoLjTE39k
=CETw
-----END PGP SIGNATURE-----

>From e67dbb6debf502cf746cc3981a0ba3c5cccd1fb5 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Sat, 7 Nov 2009 21:34:32 -0700
Subject: [PATCH] open: detect FreeBSD bug

open("link-to-file/", O_RDONLY) mistakenly succeeds.

* m4/open.m4 (gl_FUNC_OPEN): Reject FreeBSD open.
* doc/posix-functions/open.texi (open): Document the bug.
* tests/test-open.h (test_open): Add parameters, and test symlink
handling.
* tests/test-open.c (main): Adjust caller.
* tests/test-fcntl-safer.c (main): Likewise.
* modules/open-tests (Depends-on): Add stdbool, symlink.
* modules/fcntl-safer-tests (Depends-on): Likewise.
* tests/test-openat.c (main): Add test-open tests.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                     |   11 ++++++++
 doc/posix-functions/open.texi |    2 +-
 m4/open.m4                    |   17 +++++++++---
 modules/fcntl-safer-tests     |    2 +
 modules/open-tests            |    3 +-
 tests/test-fcntl-safer.c      |   20 ++++++++++++++-
 tests/test-open.c             |   20 ++++++++++++++-
 tests/test-open.h             |   56 +++++++++++++++++++++-------------------
 tests/test-openat.c           |   53 +++++++++++++++++++++++++++++++-------
 9 files changed, 139 insertions(+), 45 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ef20a64..c174a7d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2009-11-07  Eric Blake  <address@hidden>

+       open: detect FreeBSD bug
+       * m4/open.m4 (gl_FUNC_OPEN): Reject FreeBSD open.
+       * doc/posix-functions/open.texi (open): Document the bug.
+       * tests/test-open.h (test_open): Add parameters, and test symlink
+       handling.
+       * tests/test-open.c (main): Adjust caller.
+       * tests/test-fcntl-safer.c (main): Likewise.
+       * modules/open-tests (Depends-on): Add stdbool, symlink.
+       * modules/fcntl-safer-tests (Depends-on): Likewise.
+       * tests/test-openat.c (main): Add test-open tests.
+
        stat: detect FreeBSD bug
        * m4/stat.m4 (gl_FUNC_STAT): Reject FreeBSD stat.
        * doc/posix-functions/stat.texi (stat): Document the bug.
diff --git a/doc/posix-functions/open.texi b/doc/posix-functions/open.texi
index 6a919d8..933a246 100644
--- a/doc/posix-functions/open.texi
+++ b/doc/posix-functions/open.texi
@@ -12,7 +12,7 @@ open
 This function does not fail when the file name argument ends in a slash
 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.
+FreeBSD 7.2, HP-UX 11.00, Solaris 9, Irix 5.3.
 @item
 On Windows platforms (excluding Cygwin), this function does usually not
 recognize the @file{/dev/null} filename.
diff --git a/m4/open.m4 b/m4/open.m4
index c0eb8e8..ba7d876 100644
--- a/m4/open.m4
+++ b/m4/open.m4
@@ -1,4 +1,4 @@
-# open.m4 serial 7
+# open.m4 serial 8
 dnl Copyright (C) 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,
@@ -13,10 +13,15 @@ AC_DEFUN([gl_FUNC_OPEN],
       ;;
     *)
       dnl open("foo/") should not create a file when the file name has a
-      dnl trailing slash.
+      dnl trailing slash.  FreeBSD only has the problem on symlinks.
+      AC_CHECK_FUNCS_ONCE([lstat])
       AC_CACHE_CHECK([whether open recognizes a trailing slash],
         [gl_cv_func_open_slash],
-        [
+        [# Assume that if we have lstat, we can also check symlinks.
+          if test $ac_cv_func_lstat = yes; then
+            touch conftest.tmp
+            ln -s conftest.tmp conftest.lnk
+          fi
           AC_TRY_RUN([
 #include <fcntl.h>
 #if HAVE_UNISTD_H
@@ -24,18 +29,22 @@ AC_DEFUN([gl_FUNC_OPEN],
 #endif
 int main ()
 {
+#if HAVE_LSTAT
+  if (open ("conftest.lnk/", O_RDONLY) != -1) return 2;
+#endif
   return open ("conftest.sl/", O_CREAT, 0600) >= 0;
 }], [gl_cv_func_open_slash=yes], [gl_cv_func_open_slash=no],
             [
 changequote(,)dnl
              case "$host_os" in
+               freebsd*)        gl_cv_func_open_slash="guessing no" ;;
                solaris2.[0-9]*) gl_cv_func_open_slash="guessing no" ;;
                hpux*)           gl_cv_func_open_slash="guessing no" ;;
                *)               gl_cv_func_open_slash="guessing yes" ;;
              esac
 changequote([,])dnl
             ])
-          rm -f conftest.sl
+          rm -f conftest.sl conftest.tmp conftest.lnk
         ])
       case "$gl_cv_func_open_slash" in
         *no)
diff --git a/modules/fcntl-safer-tests b/modules/fcntl-safer-tests
index 6229142..3e8a2f6 100644
--- a/modules/fcntl-safer-tests
+++ b/modules/fcntl-safer-tests
@@ -3,6 +3,8 @@ tests/test-open.h
 tests/test-fcntl-safer.c

 Depends-on:
+stdbool
+symlink

 configure.ac:

diff --git a/modules/open-tests b/modules/open-tests
index 42aa93c..16d4a99 100644
--- a/modules/open-tests
+++ b/modules/open-tests
@@ -3,10 +3,11 @@ tests/test-open.h
 tests/test-open.c

 Depends-on:
+stdbool
+symlink

 configure.ac:

 Makefile.am:
 TESTS += test-open
 check_PROGRAMS += test-open
-
diff --git a/tests/test-fcntl-safer.c b/tests/test-fcntl-safer.c
index 33c7c2c..15f6e2c 100644
--- a/tests/test-fcntl-safer.c
+++ b/tests/test-fcntl-safer.c
@@ -20,6 +20,24 @@

 #include "fcntl--.h"

+#include <errno.h>
+#include <stdbool.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)
+
 #define BASE "test-fcntl-safer.t"

 #include "test-open.h"
@@ -27,5 +45,5 @@
 int
 main (void)
 {
-  return test_open ();
+  return test_open (open, true);
 }
diff --git a/tests/test-open.c b/tests/test-open.c
index 738934e..37109a5 100644
--- a/tests/test-open.c
+++ b/tests/test-open.c
@@ -20,6 +20,24 @@

 #include <fcntl.h>

+#include <errno.h>
+#include <stdbool.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)
+
 #define BASE "test-open.t"

 #include "test-open.h"
@@ -27,5 +45,5 @@
 int
 main (void)
 {
-  return test_open ();
+  return test_open (open, true);
 }
diff --git a/tests/test-open.h b/tests/test-open.h
index 4dba767..9b43945 100644
--- a/tests/test-open.h
+++ b/tests/test-open.h
@@ -16,29 +16,14 @@

 /* Written by Bruno Haible <address@hidden>, 2007.  */

-/* Include <config.h> and a form of <fcntl.h> first.  */
-
-#include <errno.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)
-
-/* Test fopen.  Assumes BASE is defined.  */
+/* This file is designed to test both open(n,buf[,mode]) and
+   openat(AT_FDCWD,n,buf[,mode]).  FUNC is the function to test.
+   Assumes that BASE and ASSERT are already defined, and that
+   appropriate headers are already included.  If PRINT, warn before
+   skipping symlink tests with status 77.  */

 static int
-test_open (void)
+test_open (int (*func) (char const *, int, ...), bool print)
 {
   int fd;
   /* Remove anything from prior partial run.  */
@@ -46,40 +31,57 @@ test_open (void)

   /* Cannot create directory.  */
   errno = 0;
-  ASSERT (open ("nonexist.ent/", O_CREAT | O_RDONLY, 0600) == -1);
+  ASSERT (func ("nonexist.ent/", O_CREAT | O_RDONLY, 0600) == -1);
   ASSERT (errno == ENOTDIR || errno == EISDIR || errno == ENOENT
           || errno == EINVAL);

   /* Create a regular file.  */
-  fd = open (BASE "file", O_CREAT | O_RDONLY, 0600);
+  fd = func (BASE "file", O_CREAT | O_RDONLY, 0600);
   ASSERT (0 <= fd);
   ASSERT (close (fd) == 0);

   /* Trailing slash handling.  */
   errno = 0;
-  ASSERT (open (BASE "file/", O_RDONLY) == -1);
+  ASSERT (func (BASE "file/", O_RDONLY) == -1);
   ASSERT (errno == ENOTDIR || errno == EISDIR || errno == EINVAL);

   /* Directories cannot be opened for writing.  */
   errno = 0;
-  ASSERT (open (".", O_WRONLY) == -1);
+  ASSERT (func (".", O_WRONLY) == -1);
   ASSERT (errno == EISDIR || errno == EACCES);

   /* /dev/null must exist, and be writable.  */
-  fd = open ("/dev/null", O_RDONLY);
+  fd = func ("/dev/null", O_RDONLY);
   ASSERT (0 <= fd);
   {
     char c;
     ASSERT (read (fd, &c, 1) == 0);
   }
   ASSERT (close (fd) == 0);
-  fd = open ("/dev/null", O_WRONLY);
+  fd = func ("/dev/null", O_WRONLY);
   ASSERT (0 <= fd);
   ASSERT (write (fd, "c", 1) == 1);
   ASSERT (close (fd) == 0);

+  /* Symlink handling, where supported.  */
+  if (symlink (BASE "file", BASE "link") != 0)
+    {
+      ASSERT (unlink (BASE "file") == 0);
+      if (print)
+        fputs ("skipping test: symlinks not supported on this file system\n",
+               stderr);
+      return 77;
+    }
+  errno = 0;
+  ASSERT (func (BASE "link/", O_RDONLY) == -1);
+  ASSERT (errno == ENOTDIR);
+  fd = func (BASE "link", O_RDONLY);
+  ASSERT (0 <= fd);
+  ASSERT (close (fd) == 0);
+
   /* Cleanup.  */
   ASSERT (unlink (BASE "file") == 0);
+  ASSERT (unlink (BASE "link") == 0);

   return 0;
 }
diff --git a/tests/test-openat.c b/tests/test-openat.c
index 6e5c519..b9b8932 100644
--- a/tests/test-openat.c
+++ b/tests/test-openat.c
@@ -20,6 +20,9 @@

 #include <fcntl.h>

+#include <errno.h>
+#include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -28,20 +31,50 @@
   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-openat.t"
+
+#include "test-open.h"
+
+static int dfd = AT_FDCWD;
+
+/* Wrapper around openat to test open behavior.  */
+static int
+do_open (char const *name, int flags, ...)
+{
+  mode_t mode = 0;
+  if (flags & O_CREAT)
+    {
+      va_list arg;
+      va_start (arg, flags);
+
+      /* We have to use PROMOTED_MODE_T instead of mode_t, otherwise GCC 4
+         creates crashing code when 'mode_t' is smaller than 'int'.  */
+      mode = va_arg (arg, PROMOTED_MODE_T);
+
+      va_end (arg);
+    }
+  return openat (dfd, name, flags, mode);
+}
+
 int
 main (void)
 {
-  /* FIXME - add more tests.  For example, share /dev/null and
-     trailing slash tests with test-open, and do more checks for
-     proper fd handling.  */
+  int result;
+
+  /* Basic checks.  */
+  result = test_open (do_open, false);
+  dfd = open (".", O_RDONLY);
+  ASSERT (0 <= dfd);
+  ASSERT (test_open (do_open, false) == result);
+  ASSERT (close (dfd) == 0);

   /* 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
@@ -49,12 +82,12 @@ main (void)
   ASSERT (close (STDIN_FILENO) == 0);
   ASSERT (openat (AT_FDCWD, ".", O_RDONLY) == STDIN_FILENO);
   {
-    int dfd = open (".", O_RDONLY);
+    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;
+  return result;
 }
-- 
1.6.5.rc1