OpenBSD chown (was: pending patches?)

[Eric Blake]

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

According to Eric Blake on 11/14/2009 12:48 PM:
> However, I'm now seeing a failure on OpenBSD, where chown refuses to
> change ctime on files if there is no change to ownership:
> 
> $ touch a
> $ gstat -c %z a
> 2009-11-14 20:06:35.175893000 +0100
> $ ktrace chgrp ericb a
> $ kdump
> ...
>  20413 chgrp    CALL  chown(0x836d6000,0xffffffff,0x3f0)
>  20413 chgrp    NAMI  "a"
>  20413 chgrp    RET   chown 0
> ...
> $ gstat -c %z a
> 2009-11-14 20:06:35.175893000 +0100
> $
> 
> So, even though the syscall is taking place, the ctime is not getting
> updated.  I confirmed that when the gid changes, ctime is indeed updating.
>  Maybe I'll have to add a call to utimensat to force the ctime, unless
> there is some easier call to force a ctime update without impacting atime
> and mtime (or at most, affecting them by truncating to microseconds due to
> missing utimensat).

It turns out that the 'no-op' chmod correctly changes ctime, and that the
"optimization" was only happening on the 'no-op' chown.  So this patch
will fix it for chown.  However, I'm stumped for lchown; OpenBSD 4.0 lacks
lutimes and lchmod, and link follows symlinks, so the only things I can
think of that modify ctime on a symlink are a double rename or flat-out
recreation with unlink/symlink.  Neither is very appealing, as it exposes
a window of time where the symlink doesn't exist.  I guess I'll just have
to find a way to modify this test to skip the portions of the lchown test
that depend on ctime when run on OpenBSD.  I'm delaying pushing this in
case anyone else has an idea, since the portion touching lchown.c is not
working...

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

iEYEARECAAYFAksCMbIACgkQ84KuGfSFAYCZuACeIMLMJruUwOuiHqNrSoROzN7d
NO4AoInvrVPbS37YiasLubpp9RGrn/uE
=ykmj
-----END PGP SIGNATURE-----

>From bee4288de8d1527a0f1234a7eac1e280400177a1 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Mon, 16 Nov 2009 14:35:41 -0700
Subject: [PATCH] chown: work around OpenBSD bug

chown(name,geteuid(),-1) failed to update the change time if
name was already owned by the current effective user.

* lib/chown.c (rpl_chown): Work around the bug.
* lib/lchown.c (rpl_lchown): Likewise.
* m4/chown.m4 (gl_FUNC_CHOWN): Test for ctime bug.
* modules/chown (Depends-on): Add stdbool.
* modules/lchown (Depends-on): Likewise.
* doc/posix-functions/chown.texi (chown): Document the bug.
* doc/posix-functions/lchown.texi (lchown): Likewise.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog                       |   11 ++++++
 doc/posix-functions/chown.texi  |    4 ++
 doc/posix-functions/lchown.texi |    4 ++
 lib/chown.c                     |   71 +++++++++++++++++++++++++++-----------
 lib/lchown.c                    |   45 ++++++++++++++++++++++--
 m4/chown.m4                     |   40 +++++++++++++++++++++-
 modules/chown                   |    3 +-
 modules/lchown                  |    1 +
 8 files changed, 151 insertions(+), 28 deletions(-)

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

+       chown: work around OpenBSD bug
+       * lib/chown.c (rpl_chown): Work around the bug.
+       * lib/lchown.c (rpl_lchown): Likewise.
+       * m4/chown.m4 (gl_FUNC_CHOWN): Test for ctime bug.
+       * modules/chown (Depends-on): Add stdbool.
+       * modules/lchown (Depends-on): Likewise.
+       * doc/posix-functions/chown.texi (chown): Document the bug.
+       * doc/posix-functions/lchown.texi (lchown): Likewise.
+
+2009-11-16  Eric Blake  <address@hidden>
+
        xalloc-die-tests: avoid printing null pointer
        * modules/xalloc-die-tests (Files, Makefile.am): Wrap execution in
        shell script.
diff --git a/doc/posix-functions/chown.texi b/doc/posix-functions/chown.texi
index e5a80d6..15aabac 100644
--- a/doc/posix-functions/chown.texi
+++ b/doc/posix-functions/chown.texi
@@ -13,6 +13,10 @@ chown
 @code{chown("link-to-file/",uid,gid)}:
 FreeBSD 7.2, Solaris 9.
 @item
+Some platforms fail to update the change time when at least one
+argument was not -1, but no ownership changes resulted:
+OpenBSD.
address@hidden
 When passed an argument of -1, some implementations really set the owner
 user/group id of the file to this value, rather than leaving that id of the
 file alone.
diff --git a/doc/posix-functions/lchown.texi b/doc/posix-functions/lchown.texi
index 0686bf3..57be673 100644
--- a/doc/posix-functions/lchown.texi
+++ b/doc/posix-functions/lchown.texi
@@ -13,6 +13,10 @@ lchown
 @code{lchown("link-to-file/",uid,gid)}:
 FreeBSD 7.2, Solaris 9.
 @item
+Some platforms fail to update the change time when at least one
+argument was not -1, but no ownership changes resulted:
+OpenBSD.
address@hidden
 This function is missing on some platforms; however, the replacement
 fails on symlinks if @code{chown} is supported, and fails altogether
 with @code{ENOSYS} otherwise:
diff --git a/lib/chown.c b/lib/chown.c
index edbccc6..66bb0c8 100644
--- a/lib/chown.c
+++ b/lib/chown.c
@@ -59,20 +59,29 @@ chown (const char *file _UNUSED_PARAMETER_, uid_t uid 
_UNUSED_PARAMETER_,
 int
 rpl_chown (const char *file, uid_t uid, gid_t gid)
 {
+  struct stat st;
+  bool stat_valid = false;
+  int result;
+
+# if CHOWN_CHANGE_TIME_BUG
+  if (gid != (gid_t) -1 || uid != (uid_t) -1)
+    {
+      if (stat (file, &st))
+        return -1;
+      stat_valid = true;
+    }
+# endif
+
 # if CHOWN_FAILS_TO_HONOR_ID_OF_NEGATIVE_ONE
   if (gid == (gid_t) -1 || uid == (uid_t) -1)
     {
-      struct stat file_stats;
-
       /* Stat file to get id(s) that should remain unchanged.  */
-      if (stat (file, &file_stats))
+      if (!stat_valid && stat (file, &st))
         return -1;
-
       if (gid == (gid_t) -1)
-        gid = file_stats.st_gid;
-
+        gid = st.st_gid;
       if (uid == (uid_t) -1)
-        uid = file_stats.st_uid;
+        uid = st.st_uid;
     }
 # endif

@@ -89,15 +98,18 @@ rpl_chown (const char *file, uid_t uid, gid_t gid)
         || (errno == EACCES
             && 0 <= (fd = open (file, O_WRONLY | open_flags))))
       {
-        int result = fchown (fd, uid, gid);
-        int saved_errno = errno;
+        int saved_errno;
+        bool fchown_socket_failure;

-        /* POSIX says fchown can fail with errno == EINVAL on sockets,
-           so fall back on chown in that case.  */
-        struct stat sb;
-        bool fchown_socket_failure =
+        result = fchown (fd, uid, gid);
+        saved_errno = errno;
+
+        /* POSIX says fchown can fail with errno == EINVAL on sockets
+           and pipes, so fall back on chown in that case.  */
+        fchown_socket_failure =
           (result != 0 && saved_errno == EINVAL
-           && fstat (fd, &sb) == 0 && S_ISFIFO (sb.st_mode));
+           && fstat (fd, &st) == 0
+           && (S_ISFIFO (st.st_mode) || S_ISSOCK (st.st_mode)));

         close (fd);

@@ -113,15 +125,32 @@ rpl_chown (const char *file, uid_t uid, gid_t gid)
 # endif

 # if CHOWN_TRAILING_SLASH_BUG
-  {
-    size_t len = strlen (file);
-    struct stat st;
-    if (len && file[len - 1] == '/' && stat (file, &st))
-      return -1;
-  }
+  if (!stat_valid)
+    {
+      size_t len = strlen (file);
+      if (len && file[len - 1] == '/' && stat (file, &st))
+        return -1;
+    }
+# endif
+
+  result = chown (file, uid, gid);
+
+# if CHOWN_CHANGE_TIME_BUG
+  if (result == 0 && stat_valid
+      && (uid == st.st_uid || uid == (uid_t) -1)
+      && (gid == st.st_gid || gid == (gid_t) -1))
+    {
+      /* No change in ownership, but at least one argument was not -1,
+         so we are required to update ctime.  Since chown succeeded,
+         we assume that chmod will do likewise.  Fortunately, on all
+         known systems where a 'no-op' chown skips the ctime update, a
+         'no-op' chmod still does the trick.  */
+      result = chmod (file, st.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO
+                                          | S_ISUID | S_ISGID | S_ISVTX));
+    }
 # endif

-  return chown (file, uid, gid);
+  return result;
 }

 #endif /* HAVE_CHOWN */
diff --git a/lib/lchown.c b/lib/lchown.c
index 265c2f7..b47b65b 100644
--- a/lib/lchown.c
+++ b/lib/lchown.c
@@ -23,6 +23,7 @@
 #include <unistd.h>

 #include <errno.h>
+#include <stdbool.h>
 #include <string.h>
 #include <sys/stat.h>

@@ -69,10 +70,46 @@ lchown (const char *file, uid_t uid, gid_t gid)
 int
 rpl_lchown (const char *file, uid_t uid, gid_t gid)
 {
-  size_t len = strlen (file);
-  if (len && file[len - 1] == '/')
-    return chown (file, uid, gid);
-  return lchown (file, uid, gid);
+  struct stat st;
+  bool stat_valid = false;
+  int result;
+
+# if CHOWN_CHANGE_TIME_BUG
+  if (gid != (gid_t) -1 || uid != (uid_t) -1)
+    {
+      if (lstat (file, &st))
+        return -1;
+      stat_valid = true;
+    }
+# endif
+
+# if CHOWN_TRAILING_SLASH_BUG
+  if (!stat_valid)
+    {
+      size_t len = strlen (file);
+      if (len && file[len - 1] == '/')
+        return chown (file, uid, gid);
+    }
+# endif
+
+  result = lchown (file, uid, gid);
+
+# if CHOWN_CHANGE_TIME_BUG
+  if (result == 0 && stat_valid
+      && (uid == st.st_uid || uid == (uid_t) -1)
+      && (gid == st.st_gid || gid == (gid_t) -1))
+    {
+      /* No change in ownership, but at least one argument was not -1,
+         so we are required to update ctime.  Since lchown succeeded,
+         we assume that lchmod will do likewise.  Fortunately, all
+         known systems where a 'no-op' lchown skips the ctime update
+         have a working lchmod, which does the trick.  */
+      result = lchmod (file, st.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO
+                                           | S_ISUID | S_ISGID | S_ISVTX));
+    }
+# endif
+
+  return result;
 }

 #endif /* HAVE_LCHOWN */
diff --git a/m4/chown.m4 b/m4/chown.m4
index 5bedfa1..0dced4b 100644
--- a/m4/chown.m4
+++ b/m4/chown.m4
@@ -1,4 +1,4 @@
-# serial 21
+# serial 22
 # Determine whether we need the chown wrapper.

 dnl Copyright (C) 1997-2001, 2003-2005, 2007, 2009
@@ -22,20 +22,27 @@ AC_DEFUN_ONCE([gl_FUNC_CHOWN],
   AC_REQUIRE([gl_FUNC_CHOWN_FOLLOWS_SYMLINK])
   AC_CHECK_FUNCS_ONCE([chown fchown])

+  dnl mingw lacks chown altogether.
   if test $ac_cv_func_chown = no; then
     HAVE_CHOWN=0
     AC_LIBOBJ([chown])
   else
+    dnl Some old systems treated chown like lchown.
     if test $gl_cv_func_chown_follows_symlink = no; then
       REPLACE_CHOWN=1
       AC_LIBOBJ([chown])
     fi
+
+    dnl Some old systems tried to use uid/gid -1 literally.
     if test $ac_cv_func_chown_works = no; then
       AC_DEFINE([CHOWN_FAILS_TO_HONOR_ID_OF_NEGATIVE_ONE], [1],
         [Define if chown is not POSIX compliant regarding IDs of -1.])
       REPLACE_CHOWN=1
       AC_LIBOBJ([chown])
     fi
+
+    dnl Solaris 9 ignores trailing slash.
+    dnl FreeBSD 7.2 mishandles trailing slash on symlinks.
     AC_CACHE_CHECK([whether chown honors trailing slash],
       [gl_cv_func_chown_slash_works],
       [touch conftest.file && rm -f conftest.link
@@ -52,10 +59,39 @@ AC_DEFUN_ONCE([gl_FUNC_CHOWN],
       rm -f conftest.link conftest.file])
     if test "$gl_cv_func_chown_slash_works" != yes; then
       AC_DEFINE([CHOWN_TRAILING_SLASH_BUG], [1],
-        [Define if chown mishandles trailing slash.])
+        [Define to 1 if chown mishandles trailing slash.])
       REPLACE_CHOWN=1
       AC_LIBOBJ([chown])
     fi
+
+    dnl OpenBSD fails to update ctime if ownership does not change.
+    AC_CACHE_CHECK([whether chown always updates ctime],
+      [gl_cv_func_chown_ctime_works],
+      [AC_RUN_IFELSE([AC_LANG_PROGRAM([[
+#include <unistd.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+]], [[    struct stat st1, st2;
+          if (close (creat ("conftest.file", 0600))) return 1;
+          if (stat ("conftest.file", &st1)) return 2;
+          sleep (1);
+          if (chown ("conftest.file", st1.st_uid, st1.st_gid)) return 3;
+          if (stat ("conftest.file", &st2)) return 4;
+          if (st2.st_ctime <= st1.st_ctime) return 5;
+        ]])],
+        [gl_cv_func_chown_ctime_works=yes],
+        [gl_cv_func_chown_ctime_works=no],
+        [gl_cv_func_chown_ctime_works="guessing no"])
+      rm -f conftest.file])
+    if test "$gl_cv_func_chown_ctime_works" != yes; then
+      AC_DEFINE([CHOWN_CHANGE_TIME_BUG], [1], [Define to 1 if chown fails
+        to change ctime when at least one argument was not -1.])
+      REPLACE_CHOWN=1
+      AC_LIBOBJ([chown])
+    fi
+
     if test $REPLACE_CHOWN = 1 && test $ac_cv_func_fchown = no; then
       AC_LIBOBJ([fchown-stub])
     fi
diff --git a/modules/chown b/modules/chown
index 88d0cd4..4c296ac 100644
--- a/modules/chown
+++ b/modules/chown
@@ -8,9 +8,10 @@ m4/chown.m4

 Depends-on:
 open
-unistd
 stat
+stdbool
 sys_stat
+unistd

 configure.ac:
 gl_FUNC_CHOWN
diff --git a/modules/lchown b/modules/lchown
index 233e334..75672b4 100644
--- a/modules/lchown
+++ b/modules/lchown
@@ -9,6 +9,7 @@ Depends-on:
 chown
 errno
 lstat
+stdbool
 sys_stat
 unistd

-- 
1.6.5.rc1