Re: Bug#249177: coreutils: chown is not POSIXLY_CORRECT even when the variable is set

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> Shouldn't we should either deprecate and undocument that option or
> make it work once again?

Either is fine with me.  Here's a proposed patch to make it work.  I
noticed some other bugs in this area (also in chmod) and this patch
fixes them too.

2004-06-08  Paul Eggert  <address@hidden>

        Adjust chmod and chown to be similar if -c or -v are given.  In
        particular, a no-op chown is no longer reported as a change; this
        reverts to previous behavior.  Also, fix both commands so that -v
        report failures even if the failure is not due to the chmod or
        chown syscalls.

        * src/chmod.c (CH_NOT_APPLIED): New constant.
        (describe_change): Handle it.
        (process_file): Use it, if a symlink wasn't changed.
        (mode_changed): Return bool, not int.  Accept new argument
        NEW_MODE; all callers changed.  This lets us avoid statting the
        file unless the new mode has unusual bits.
        (process_file): Return -1 on error.  With -v, report all errors
        verbosely, not just some.

        * src/chown-core.c (change_file_owner): Return -1 on error, not
        1 sometimes and -1 on others.  Our caller ORs together our results,
        and (-1 | 1) == 0 on ones-complement hosts.
        With -v report all errors verbosely, not just some.
        Fix bug when chopt->root_dev_ino && !chopt->affect_symlink_referent:
        file_stats wasn't set properly in that case.

        * tests/chgrp/basic: Adjust to above changes.

Index: src/chmod.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/chmod.c,v
retrieving revision 1.101
diff -p -u -r1.101 chmod.c
--- src/chmod.c 27 Mar 2004 08:43:36 -0000      1.101
+++ src/chmod.c 8 Jun 2004 22:07:23 -0000
@@ -39,6 +39,7 @@
 
 enum Change_status
 {
+  CH_NOT_APPLIED,
   CH_SUCCEEDED,
   CH_FAILED,
   CH_NO_CHANGE_REQUESTED
@@ -100,19 +101,30 @@ static struct option const long_options[
   {0, 0, 0, 0}
 };
 
-static int
-mode_changed (const char *file, mode_t old_mode)
-{
-  struct stat new_stats;
+/* Return true if the chmodable permission bits of FILE changed.
+   The old mode was OLD_MODE, but it was changed to NEW_MODE.  */
 
-  if (stat (file, &new_stats))
+static bool
+mode_changed (char const *file, mode_t old_mode, mode_t new_mode)
+{
+  if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
     {
-      if (force_silent == 0)
-       error (0, errno, _("getting new attributes of %s"), quote (file));
-      return 0;
+      /* The new mode contains unusual bits that the call to chmod may
+        have silently cleared.  Check whether they actually changed.  */
+
+      struct stat new_stats;
+
+      if (stat (file, &new_stats) != 0)
+       {
+         if (!force_silent)
+           error (0, errno, _("getting new attributes of %s"), quote (file));
+         return 0;
+       }
+
+      new_mode = new_stats.st_mode;
     }
 
-  return old_mode != new_stats.st_mode;
+  return ((old_mode ^ new_mode) & CHMOD_MODE_BITS) != 0;
 }
 
 /* Tell the user how/if the MODE of FILE has been changed.
@@ -125,6 +137,13 @@ describe_change (const char *file, mode_
   char perms[11];              /* "-rwxrwxrwx" ls-style modes. */
   const char *fmt;
 
+  if (changed == CH_NOT_APPLIED)
+    {
+      printf (_("neither symbolic link %s nor referent has been changed\n"),
+             quote (file));
+      return;
+    }
+
   mode_string (mode, perms);
   perms[10] = '\0';            /* `mode_string' does not null terminate. */
   switch (changed)
@@ -146,70 +165,85 @@ describe_change (const char *file, mode_
 }
 
 /* Change the mode of FILE according to the list of operations CHANGES.
-   Return 0 if successful, 1 if errors occurred.  This function is called
+   Return 0 if successful, -1 if errors occurred.  This function is called
    once for every file system object that fts encounters.  */
 
 static int
 process_file (FTS *fts, FTSENT *ent, const struct mode_change *changes)
 {
-  const char *file_full_name = ent->fts_path;
-  const char *file = ent->fts_accpath;
-  const struct stat *sb = ent->fts_statp;
-  mode_t newmode;
+  char const *file_full_name = ent->fts_path;
+  char const *file = ent->fts_accpath;
+  const struct stat *file_stats = ent->fts_statp;
+  mode_t new_mode IF_LINT (= 0);
   int errors = 0;
-  int fail;
-  int saved_errno;
+  bool do_chmod;
+  bool symlink_changed = true;
 
   switch (ent->fts_info)
     {
+    case FTS_DP:
+      return 0;
+
     case FTS_NS:
       error (0, ent->fts_errno, _("cannot access %s"), quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     case FTS_ERR:
       error (0, ent->fts_errno, _("%s"), quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     case FTS_DNR:
       error (0, ent->fts_errno, _("cannot read directory %s"),
             quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     default:
       break;
     }
 
-  /* If this is the second (post-order) encounter with a directory,
-     then return right away.  */
-  if (ent->fts_info == FTS_DP)
-    return 0;
+  do_chmod = !errors;
 
-  if (ROOT_DEV_INO_CHECK (root_dev_ino, sb))
+  if (do_chmod && ROOT_DEV_INO_CHECK (root_dev_ino, file_stats))
     {
       ROOT_DEV_INO_WARN (file_full_name);
-      return 1;
+      errors = -1;
+      do_chmod = false;
     }
 
-  if (S_ISLNK (sb->st_mode))
-    return 0;
-
-  newmode = mode_adjust (sb->st_mode, changes);
-
-  fail = chmod (file, newmode);
-  saved_errno = errno;
+  if (do_chmod)
+    {
+      new_mode = mode_adjust (file_stats->st_mode, changes);
 
-  if (verbosity == V_high
-      || (verbosity == V_changes_only
-         && !fail && mode_changed (file, sb->st_mode)))
-    describe_change (file_full_name, newmode,
-                    (fail ? CH_FAILED : CH_SUCCEEDED));
+      if (S_ISLNK (file_stats->st_mode))
+       symlink_changed = false;
+      else
+       {
+         errors = chmod (file, new_mode);
+
+         if (errors && !force_silent)
+           error (0, errno, _("changing permissions of %s"),
+                  quote (file_full_name));
+       }
+    }
 
-  if (fail)
+  if (verbosity != V_off)
     {
-      if (force_silent == 0)
-       error (0, saved_errno, _("changing permissions of %s"),
-              quote (file_full_name));
-      errors = 1;
+      bool changed =
+       (!errors && symlink_changed
+        && mode_changed (file, file_stats->st_mode, new_mode));
+
+      if (changed || verbosity == V_high)
+       {
+         enum Change_status ch_status =
+           (errors ? CH_FAILED
+            : !symlink_changed ? CH_NOT_APPLIED
+            : !changed ? CH_NO_CHANGE_REQUESTED
+            : CH_SUCCEEDED);
+         describe_change (file_full_name, new_mode, ch_status);
+       }
     }
 
   if ( ! recurse)
Index: src/chown-core.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/chown-core.c,v
retrieving revision 1.24
diff -p -u -r1.24 chown-core.c
--- src/chown-core.c    8 Jun 2004 14:57:57 -0000       1.24
+++ src/chown-core.c    8 Jun 2004 21:56:50 -0000
@@ -167,18 +167,19 @@ describe_change (const char *file, enum 
    to UID and/or GID as appropriate.
    FIXME: describe old_uid and old_gid.
    CHOPT specifies additional options.
-   Return nonzero upon error, zero otherwise.  */
+   Return 0 if successful, -1 if errors occurred.  */
 static int
 change_file_owner (FTS *fts, FTSENT *ent,
                   uid_t uid, gid_t gid,
                   uid_t old_uid, gid_t old_gid,
                   struct Chown_option const *chopt)
 {
-  const char *file_full_name = ent->fts_path;
+  char const *file_full_name = ent->fts_path;
+  char const *file = ent->fts_accpath;
   struct stat const *file_stats IF_LINT (= NULL);
   struct stat stat_buf;
   int errors = 0;
-  bool do_chown = true;
+  bool do_chown;
   bool symlink_changed = true;
 
   switch (ent->fts_info)
@@ -195,23 +196,30 @@ change_file_owner (FTS *fts, FTSENT *ent
 
     case FTS_NS:
       error (0, ent->fts_errno, _("cannot access %s"), quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     case FTS_ERR:
       error (0, ent->fts_errno, _("%s"), quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     case FTS_DNR:
       error (0, ent->fts_errno, _("cannot read directory %s"),
             quote (file_full_name));
-      return 1;
+      errors = -1;
+      break;
 
     default:
       break;
     }
 
-  if (old_uid != (uid_t) -1 || old_gid != (gid_t) -1
-      || (chopt->root_dev_ino && chopt->affect_symlink_referent))
+  if (errors)
+    do_chown = false;
+  else if (old_uid == (uid_t) -1 && old_gid == (gid_t) -1
+          && chopt->verbosity == V_off && ! chopt->root_dev_ino)
+    do_chown = true;
+  else
     {
       file_stats = ent->fts_statp;
 
@@ -219,32 +227,30 @@ change_file_owner (FTS *fts, FTSENT *ent
         stat it to get the permissions of the referent.  */
       if (S_ISLNK (file_stats->st_mode) && chopt->affect_symlink_referent)
        {
-         if (stat (ent->fts_accpath, &stat_buf) != 0)
+         if (stat (file, &stat_buf) != 0)
            {
              error (0, errno, _("cannot dereference %s"),
                     quote (file_full_name));
-             return 1;
+             errors = -1;
            }
 
          file_stats = &stat_buf;
        }
 
-      do_chown = ((old_uid == (uid_t) -1
-                  || file_stats->st_uid == old_uid)
-                 && (old_gid == (gid_t) -1
-                     || file_stats->st_gid == old_gid));
+      do_chown = (!errors
+                 && (old_uid == (uid_t) -1 || old_uid == file_stats->st_uid)
+                 && (old_gid == (gid_t) -1 || old_gid == file_stats->st_gid));
     }
 
-  if (do_chown)
+  if (do_chown && ROOT_DEV_INO_CHECK (chopt->root_dev_ino, file_stats))
     {
-      const char *file = ent->fts_accpath;
-
-      if (ROOT_DEV_INO_CHECK (chopt->root_dev_ino, file_stats))
-       {
-         ROOT_DEV_INO_WARN (file_full_name);
-         return 1;
-       }
+      ROOT_DEV_INO_WARN (file_full_name);
+      errors = -1;
+      do_chown = false;
+    }
 
+  if (do_chown)
+    {
       if (chopt->affect_symlink_referent)
        {
          /* Applying chown to a symlink and expecting it to affect
@@ -280,15 +286,23 @@ change_file_owner (FTS *fts, FTSENT *ent
               quote (file_full_name));
     }
 
-  if (chopt->verbosity == V_high
-      || (chopt->verbosity == V_changes_only && !errors))
+  if (chopt->verbosity != V_off)
     {
-      enum Change_status ch_status = (!do_chown ? CH_NO_CHANGE_REQUESTED
-                                     : !symlink_changed ? CH_NOT_APPLIED
-                                     : errors ? CH_FAILED
-                                     : CH_SUCCEEDED);
-      describe_change (file_full_name, ch_status,
-                      chopt->user_name, chopt->group_name);
+      bool changed =
+       (do_chown && !errors && symlink_changed
+        && ! ((uid == (uid_t) -1 || uid == file_stats->st_uid)
+              && (gid == (gid_t) -1 || gid == file_stats->st_gid)));
+
+      if (changed || chopt->verbosity == V_high)
+       {
+         enum Change_status ch_status =
+           (errors ? CH_FAILED
+            : !symlink_changed ? CH_NOT_APPLIED
+            : !changed ? CH_NO_CHANGE_REQUESTED
+            : CH_SUCCEEDED);
+         describe_change (file_full_name, ch_status,
+                          chopt->user_name, chopt->group_name);
+       }
     }
 
   if ( ! chopt->recurse)
@@ -315,9 +329,8 @@ chown_files (char **files, int bit_flags
   int fail = 0;
 
   /* Use lstat and stat only if they're needed.  */
-  int stat_flags = ((chopt->root_dev_ino
-                    || required_uid != (uid_t) -1
-                    || required_gid != (gid_t) -1)
+  int stat_flags = ((required_uid != (uid_t) -1 || required_gid != (gid_t) -1
+                    || chopt->verbosity != V_off || chopt->root_dev_ino)
                    ? 0
                    : FTS_NOSTAT);
 
Index: tests/chgrp/basic
===================================================================
RCS file: /home/meyering/coreutils/cu/tests/chgrp/basic,v
retrieving revision 1.16
diff -p -u -r1.16 basic
--- tests/chgrp/basic   8 Jun 2004 16:58:28 -0000       1.16
+++ tests/chgrp/basic   8 Jun 2004 19:38:38 -0000
@@ -87,9 +87,8 @@ test "$VERBOSE" = yes && set +x
 cat <<\EOF > expected
 changed group of `f' to G1
 changed group of `f' to G2
-changed group of `f' to G2
-changed group of `f' to G1
 changed group of `f' to G1
+group of `f' retained as G1
 changed group of `f' to G2
 changed group of `d/f3' to G2
 changed group of `d' to G2
@@ -100,7 +99,6 @@ changed group of `d' to G2
 changed group of `d/f3' to G1
 changed group of `d' to G1
 changed group of `d' to G2
-changed group of `symlink' to G2
 changed ownership of `f' to :G2
 changed group of `symlink' to G1
 changed ownership of `f' to :G2