fchmod-tests, fchmodat tests, lchmod tests: Add more tests

fchmod-tests, fchmodat tests, lchmod tests: Add more tests

[Bruno Haible]

It smells like additional tests for fchmod(), fchmodat(), lchmod() could
uncover bugs on some platforms. So I'm adding more tests.


2021-01-09  Bruno Haible  <bruno@clisp.org>

	fchmod-tests, fchmodat tests, lchmod tests: Add more tests.
	* tests/test-fchmod.c: Include <fcntl.h>.
	(BASE): New macro.
	(main): Add more tests.
	* tests/test-fchmodat.c (main): Add more tests.
	* tests/test-lchmod.c (main): Likewise.

diff --git a/tests/test-fchmod.c b/tests/test-fchmod.c
index 43b70ac..f8db9e5 100644
--- a/tests/test-fchmod.c
+++ b/tests/test-fchmod.c
@@ -22,10 +22,13 @@
 SIGNATURE_CHECK (fchmod, int, (int, mode_t));
 
 #include <errno.h>
+#include <fcntl.h>
 #include <unistd.h>
 
 #include "macros.h"
 
+#define BASE "test-fchmod."
+
 int
 main (void)
 {
@@ -42,5 +45,24 @@ main (void)
     ASSERT (errno == EBADF);
   }
 
+  /* Test that fchmod works on regular files.  */
+  {
+    struct stat statbuf;
+    int fd;
+
+    unlink (BASE "file");
+    ASSERT (close (creat (BASE "file", 0600)) == 0);
+    fd = open (BASE "file", O_RDWR);
+    ASSERT (fd >= 0);
+    ASSERT (fchmod (fd, 0400) == 0);
+    ASSERT (stat (BASE "file", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0400);
+    ASSERT (close (fd) == 0);
+
+    /* Clean up.  */
+    ASSERT (chmod (BASE "file", 0600) == 0);
+    ASSERT (unlink (BASE "file") == 0);
+  }
+
   return 0;
 }
diff --git a/tests/test-fchmodat.c b/tests/test-fchmodat.c
index 5e4d2b0..6d430fa 100644
--- a/tests/test-fchmodat.c
+++ b/tests/test-fchmodat.c
@@ -46,6 +46,39 @@ main (void)
     ASSERT (errno == EBADF);
   }
 
+  /* Test that fchmodat works on regular files.  */
+  {
+    struct stat statbuf;
+
+    unlink (BASE "file");
+    ASSERT (close (creat (BASE "file", 0600)) == 0);
+    ASSERT (fchmodat (AT_FDCWD, BASE "file", 0400, 0) == 0);
+    ASSERT (stat (BASE "file", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0400);
+
+    errno = 0;
+    ASSERT (fchmodat (AT_FDCWD, BASE "file/", 0600, 0) == -1);
+    ASSERT (errno == ENOTDIR);
+
+    /* Clean up.  */
+    ASSERT (chmod (BASE "file", 0600) == 0);
+    ASSERT (unlink (BASE "file") == 0);
+  }
+
+  /* Test that fchmodat works on directories.  */
+  {
+    struct stat statbuf;
+    rmdir (BASE "dir");
+    ASSERT (mkdir (BASE "dir", 0700) == 0);
+    ASSERT (fchmodat (AT_FDCWD, BASE "dir", 0500, 0) == 0);
+    ASSERT (stat (BASE "dir", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0500);
+    ASSERT (fchmodat (AT_FDCWD, BASE "dir/", 0700, 0) == 0);
+
+    /* Clean up.  */
+    ASSERT (rmdir (BASE "dir") == 0);
+  }
+
   /* Test that fchmodat works on non-symlinks, when given
      the AT_SYMLINK_NOFOLLOW flag.  */
   {
diff --git a/tests/test-lchmod.c b/tests/test-lchmod.c
index 7b8e8a1..d4c811e 100644
--- a/tests/test-lchmod.c
+++ b/tests/test-lchmod.c
@@ -33,7 +33,7 @@ SIGNATURE_CHECK (lchmod, int, (const char *, mode_t));
 int
 main (void)
 {
-  /* Test that lchmod works on non-symlinks.  */
+  /* Test that lchmod works on regular files.  */
   {
     struct stat statbuf;
     unlink (BASE "file");
@@ -41,11 +41,31 @@ main (void)
     ASSERT (lchmod (BASE "file", 0400) == 0);
     ASSERT (stat (BASE "file", &statbuf) >= 0);
     ASSERT ((statbuf.st_mode & 0700) == 0400);
+
+    errno = 0;
+    ASSERT (lchmod (BASE "file/", 0600) == -1);
+    ASSERT (errno == ENOTDIR);
+
     /* Clean up.  */
     ASSERT (chmod (BASE "file", 0600) == 0);
     ASSERT (unlink (BASE "file") == 0);
   }
 
+  /* Test that lchmod works on directories.  */
+  {
+    struct stat statbuf;
+
+    rmdir (BASE "dir");
+    ASSERT (mkdir (BASE "dir", 0700) == 0);
+    ASSERT (lchmod (BASE "dir", 0500) == 0);
+    ASSERT (stat (BASE "dir", &statbuf) >= 0);
+    ASSERT ((statbuf.st_mode & 0700) == 0500);
+    ASSERT (lchmod (BASE "dir/", 0700) == 0);
+
+    /* Clean up.  */
+    ASSERT (rmdir (BASE "dir") == 0);
+  }
+
   /* Test that lchmod on symlinks does not modify the symlink target.  */
   {
     unlink (BASE "file");

Re: fchmod-tests, fchmodat tests, lchmod tests: Add more tests

[Bruno Haible]

I wrote:
> It smells like additional tests for fchmod(), fchmodat(), lchmod() could
> uncover bugs on some platforms. So I'm adding more tests.

And indeed, on AIX 7.2 one of these new tests fails:

../../gltests/test-fchmodat.c:60: assertion 'fchmodat (AT_FDCWD, BASE "file/", 0600, 0) == -1' failed
FAIL test-fchmodat (exit status: 134)

This patch provides a workaround.


2021-01-09  Bruno Haible  <bruno@clisp.org>

	fchmodat: Work around trailing slash bug in fchmodat() on AIX 7.2.
	* m4/fchmodat.m4 (gl_FUNC_FCHMODAT): In the test whether fchmodat works,
	also test for the trailing slashes behaviour. Define
	HAVE_NEARLY_WORKING_FCHMODAT if this is the only missing feature.
	* lib/fchmodat.c (fchmodat): If HAVE_NEARLY_WORKING_FCHMODAT, handle
	trailing slashes here.
	* modules/fchmodat (Depends-on): Sort by condition, not alphabetically.
	* doc/posix-functions/fchmodat.texi: Document the AIX bug.

diff --git a/doc/posix-functions/fchmodat.texi b/doc/posix-functions/fchmodat.texi
index 10821b6..aa0c0fd 100644
--- a/doc/posix-functions/fchmodat.texi
+++ b/doc/posix-functions/fchmodat.texi
@@ -14,6 +14,10 @@ glibc 2.3.6, Mac OS X 10.5, FreeBSD 6.0, NetBSD 5.0, OpenBSD 3.8, Minix 3.1.8,
 AIX 5.1, HP-UX 11, IRIX 6.5, Solaris 10, Cygwin 1.5.x, mingw, MSVC 14.
 But the replacement function is not safe to be used in libraries and is not multithread-safe.
 @item
+This function does not fail when the file name argument ends in a slash
+and (without the slash) names a non-directory, on some platforms:
+AIX 7.2.
+@item
 When given the @code{AT_SYMLINK_NOFOLLOW} flag,
 this function fails with @code{errno} set to @code{ENOTSUP},
 even when the file is not a symbolic link:
diff --git a/lib/fchmodat.c b/lib/fchmodat.c
index d27c0d7..eb6e224 100644
--- a/lib/fchmodat.c
+++ b/lib/fchmodat.c
@@ -38,6 +38,7 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 #include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 
 #ifdef __osf__
@@ -63,6 +64,22 @@ orig_fchmodat (int dir, char const *file, mode_t mode, int flags)
 int
 fchmodat (int dir, char const *file, mode_t mode, int flags)
 {
+# if HAVE_NEARLY_WORKING_FCHMODAT
+  /* Correct the trailing slash handling.  */
+  size_t len = strlen (file);
+  if (len && file[len - 1] == '/')
+    {
+      struct stat st;
+      if (fstatat (dir, file, &st, flags & AT_SYMLINK_NOFOLLOW) < 0)
+        return -1;
+      if (!S_ISDIR (st.st_mode))
+        {
+          errno = ENOTDIR;
+          return -1;
+        }
+    }
+# endif
+
 # if NEED_FCHMODAT_NONSYMLINK_FIX
   if (flags == AT_SYMLINK_NOFOLLOW)
     {
diff --git a/m4/fchmodat.m4 b/m4/fchmodat.m4
index 0938032..66c0e30 100644
--- a/m4/fchmodat.m4
+++ b/m4/fchmodat.m4
@@ -1,4 +1,4 @@
-# fchmodat.m4 serial 5
+# fchmodat.m4 serial 6
 dnl Copyright (C) 2004-2021 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -16,11 +16,9 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
     HAVE_FCHMODAT=0
   else
     AC_CACHE_CHECK(
-      [whether fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks],
+      [whether fchmodat works],
       [gl_cv_func_fchmodat_works],
-      [dnl This test fails on GNU/Linux with glibc 2.31 (but not on
-       dnl GNU/kFreeBSD nor GNU/Hurd) and Cygwin 2.9.
-       AC_RUN_IFELSE(
+      [AC_RUN_IFELSE(
          [AC_LANG_PROGRAM(
             [
               AC_INCLUDES_DEFAULT[
@@ -44,27 +42,49 @@ AC_DEFUN([gl_FUNC_FCHMODAT],
             [[
               int permissive = S_IRWXU | S_IRWXG | S_IRWXO;
               int desired = S_IRUSR | S_IWUSR;
-              static char const f[] = "conftest.fchmodat";
+              int result = 0;
+              #define file "conftest.fchmodat"
               struct stat st;
-              if (creat (f, permissive) < 0)
+              if (creat (file, permissive) < 0)
                 return 1;
-              if (fchmodat (AT_FDCWD, f, desired, AT_SYMLINK_NOFOLLOW) != 0)
+              /* Test whether fchmodat rejects a trailing slash on a non-directory.
+                 This test fails on AIX 7.2.  */
+              if (fchmodat (AT_FDCWD, file "/", desired, 0) == 0)
+                result |= 2;
+              /* Test whether fchmodat+AT_SYMLINK_NOFOLLOW works on non-symlinks.
+                 This test fails on GNU/Linux with glibc 2.31 (but not on
+                 GNU/kFreeBSD nor GNU/Hurd) and Cygwin 2.9.  */
+              if (fchmodat (AT_FDCWD, file, desired, AT_SYMLINK_NOFOLLOW) != 0)
+                result |= 4;
+              if (stat (file, &st) != 0)
                 return 1;
-              if (stat (f, &st) != 0)
-                return 1;
-              return ! ((st.st_mode & permissive) == desired);
+              if ((st.st_mode & permissive) != desired)
+                result |= 4;
+              return result;
             ]])],
          [gl_cv_func_fchmodat_works=yes],
-         [gl_cv_func_fchmodat_works=no],
+         [case $? in
+            2) gl_cv_func_fchmodat_works='nearly' ;;
+            *) gl_cv_func_fchmodat_works=no ;;
+          esac
+         ],
          [case "$host_os" in
-            dnl Guess no on Linux with glibc and Cygwin, yes otherwise.
+                                  # Guess no on Linux with glibc and Cygwin.
             linux-gnu* | cygwin*) gl_cv_func_fchmodat_works="guessing no" ;;
+                                  # Guess 'nearly' on AIX.
+            aix*)                 gl_cv_func_fchmodat_works="guessing nearly" ;;
+                                  # If we don't know, obey --enable-cross-guesses.
             *)                    gl_cv_func_fchmodat_works="$gl_cross_guess_normal" ;;
           esac
          ])
        rm -f conftest.fchmodat])
-    case $gl_cv_func_fchmodat_works in
+    case "$gl_cv_func_fchmodat_works" in
       *yes) ;;
+      *nearly)
+        AC_DEFINE([HAVE_NEARLY_WORKING_FCHMODAT], [1],
+          [Define to 1 if fchmodat works, except for the trailing slash handling.])
+        REPLACE_FCHMODAT=1
+        ;;
       *)
         AC_DEFINE([NEED_FCHMODAT_NONSYMLINK_FIX], [1],
           [Define to 1 if fchmodat+AT_SYMLINK_NOFOLLOW does not work right on non-symlinks.])
diff --git a/modules/fchmodat b/modules/fchmodat
index 975889b..17cfe39 100644
--- a/modules/fchmodat
+++ b/modules/fchmodat
@@ -9,20 +9,20 @@ m4/fchmodat.m4
 Depends-on:
 sys_stat
 extensions
-at-internal     [test $HAVE_FCHMODAT = 0]
-c99             [test $REPLACE_FCHMODAT = 1]
 errno           [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+fcntl-h         [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+unistd          [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+intprops        [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
+c99             [test $REPLACE_FCHMODAT = 1]
+fstatat         [test $REPLACE_FCHMODAT = 1]
+at-internal     [test $HAVE_FCHMODAT = 0]
 extern-inline   [test $HAVE_FCHMODAT = 0]
 fchdir          [test $HAVE_FCHMODAT = 0]
-fcntl-h         [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
 filename        [test $HAVE_FCHMODAT = 0]
-fstatat         [test $REPLACE_FCHMODAT = 1]
-intprops        [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
 lchmod          [test $HAVE_FCHMODAT = 0]
 openat-die      [test $HAVE_FCHMODAT = 0]
 openat-h        [test $HAVE_FCHMODAT = 0]
 save-cwd        [test $HAVE_FCHMODAT = 0]
-unistd          [test $HAVE_FCHMODAT = 0 || test $REPLACE_FCHMODAT = 1]
 
 configure.ac:
 gl_FUNC_FCHMODAT