Subject: bug#50070: closed (Re: bug#50070: chmod reads uninitialized variables when fts_info is an error and -v is set, leading to random error messages)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 16 Aug 2021 04:34:02 +0000

Your bug report

#50070: chmod reads uninitialized variables when fts_info is an error and -v is set, leading to random error messages

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
50070: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=50070
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#50070: chmod reads uninitialized variables when fts_info is an error and -v is set, leading to random error messages
From: Paul Eggert <[email-masked]>
To: Michael Debertol <[email-masked]>
Cc: [email-masked]
Date: Sun, 15 Aug 2021 21:33:26 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

Thanks for reporting the bug. I installed the attached patch to fix it.
>From af4711d5dc15f0c014bfd9c92f4eadedb89f732f Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sun, 15 Aug 2021 21:29:38 -0700
Subject: [PATCH] chmod: fix use of uninitialized var if -v

Problem reported by Michael Debertol (Bug#50070).
* NEWS: Mention the fix.
* src/chmod.c (struct change_status): New struct, replacing the
old enum Change_status.  All uses changed.
(describe_change): Distinguish between cases depending on
whether 'stat' or its equivalent succeeded.  Report a line
of output even if 'stat' failed, as that matches the documentation.
Rework to avoid casts.
(process_file): Do not output nonsense modes computed from
uninitialized storage, removing a couple of IF_LINTs.  Simplify by
defaulting to CH_NO_STAT.
---
 NEWS        |   3 ++
 src/chmod.c | 127 ++++++++++++++++++++++++++--------------------------
 2 files changed, 67 insertions(+), 63 deletions(-)

diff --git a/NEWS b/NEWS
index e14de1397..ddec56bdf 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  chmod -v no longer misreports modes of dangling symlinks.
+  [bug introduced in coreutils-5.3.0]
+
   cp -a --attributes-only now never removes destination files,
   even if the destination files are hardlinked, or the source
   is a non regular file.
diff --git a/src/chmod.c b/src/chmod.c
index 160a0c537..4447e784e 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -39,12 +39,19 @@
   proper_name ("David MacKenzie"), \
   proper_name ("Jim Meyering")
 
-enum Change_status
+struct change_status
 {
-  CH_NOT_APPLIED,
-  CH_SUCCEEDED,
-  CH_FAILED,
-  CH_NO_CHANGE_REQUESTED
+  enum
+    {
+      CH_NO_STAT,
+      CH_NOT_APPLIED,
+      CH_FAILED,
+      CH_NO_CHANGE_REQUESTED,
+      CH_SUCCEEDED
+    }
+    status;
+  mode_t old_mode;
+  mode_t new_mode;
 };
 
 enum Verbosity
@@ -136,30 +143,42 @@ mode_changed (int dir_fd, char const *file, char const *file_full_name,
 }
 
 /* Tell the user how/if the MODE of FILE has been changed.
-   CHANGED describes what (if anything) has happened. */
+   CH describes what (if anything) has happened. */
 
 static void
-describe_change (char const *file, mode_t old_mode, mode_t mode,
-                 enum Change_status changed)
+describe_change (char const *file, struct change_status const *ch)
 {
   char perms[12];		/* "-rwxrwxrwx" ls-style modes. */
   char old_perms[12];
   char const *fmt;
+  char const *quoted_file = quoteaf (file);
 
-  if (changed == CH_NOT_APPLIED)
+  switch (ch->status)
     {
+    case CH_NOT_APPLIED:
       printf (_("neither symbolic link %s nor referent has been changed\n"),
-              quoteaf (file));
+              quoted_file);
       return;
-    }
 
-  strmode (mode, perms);
+    case CH_NO_STAT:
+      printf (_("%s could not be accessed\n"), quoted_file);
+      return;
+
+    default:
+      break;
+  }
+
+  unsigned long int
+    old_m = ch->old_mode & CHMOD_MODE_BITS,
+    m = ch->new_mode & CHMOD_MODE_BITS;
+
+  strmode (ch->new_mode, perms);
   perms[10] = '\0';		/* Remove trailing space.  */
 
-  strmode (old_mode, old_perms);
+  strmode (ch->old_mode, old_perms);
   old_perms[10] = '\0';		/* Remove trailing space.  */
 
-  switch (changed)
+  switch (ch->status)
     {
     case CH_SUCCEEDED:
       fmt = _("mode of %s changed from %04lo (%s) to %04lo (%s)\n");
@@ -169,15 +188,12 @@ describe_change (char const *file, mode_t old_mode, mode_t mode,
       break;
     case CH_NO_CHANGE_REQUESTED:
       fmt = _("mode of %s retained as %04lo (%s)\n");
-      printf (fmt, quoteaf (file),
-              (unsigned long int) (mode & CHMOD_MODE_BITS), &perms[1]);
+      printf (fmt, quoted_file, m, &perms[1]);
       return;
     default:
       abort ();
     }
-  printf (fmt, quoteaf (file),
-          (unsigned long int) (old_mode & CHMOD_MODE_BITS), &old_perms[1],
-          (unsigned long int) (mode & CHMOD_MODE_BITS), &perms[1]);
+  printf (fmt, quoted_file, old_m, &old_perms[1], m, &perms[1]);
 }
 
 /* Change the mode of FILE.
@@ -190,10 +206,8 @@ process_file (FTS *fts, FTSENT *ent)
   char const *file_full_name = ent->fts_path;
   char const *file = ent->fts_accpath;
   const struct stat *file_stats = ent->fts_statp;
-  mode_t old_mode IF_LINT ( = 0);
-  mode_t new_mode IF_LINT ( = 0);
-  bool ok = true;
-  bool chmod_succeeded = false;
+  struct change_status ch;
+  ch.status = CH_NO_STAT;
 
   switch (ent->fts_info)
     {
@@ -217,27 +231,23 @@ process_file (FTS *fts, FTSENT *ent)
       if (! force_silent)
         error (0, ent->fts_errno, _("cannot access %s"),
                quoteaf (file_full_name));
-      ok = false;
       break;
 
     case FTS_ERR:
       if (! force_silent)
         error (0, ent->fts_errno, "%s", quotef (file_full_name));
-      ok = false;
       break;
 
     case FTS_DNR:
       if (! force_silent)
         error (0, ent->fts_errno, _("cannot read directory %s"),
                quoteaf (file_full_name));
-      ok = false;
       break;
 
     case FTS_SLNONE:
       if (! force_silent)
         error (0, 0, _("cannot operate on dangling symlink %s"),
                quoteaf (file_full_name));
-      ok = false;
       break;
 
     case FTS_DC:		/* directory that causes cycles */
@@ -246,13 +256,14 @@ process_file (FTS *fts, FTSENT *ent)
           emit_cycle_warning (file_full_name);
           return false;
         }
-      break;
-
+      FALLTHROUGH;
     default:
+      ch.status = CH_NOT_APPLIED;
       break;
     }
 
-  if (ok && ROOT_DEV_INO_CHECK (root_dev_ino, file_stats))
+  if (ch.status == CH_NOT_APPLIED
+      && ROOT_DEV_INO_CHECK (root_dev_ino, file_stats))
     {
       ROOT_DEV_INO_WARN (file_full_name);
       /* Tell fts not to traverse into this hierarchy.  */
@@ -262,66 +273,56 @@ process_file (FTS *fts, FTSENT *ent)
       return false;
     }
 
-  if (ok)
+  if (ch.status == CH_NOT_APPLIED && ! S_ISLNK (file_stats->st_mode))
     {
-      old_mode = file_stats->st_mode;
-      new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
-                              change, NULL);
-
-      if (! S_ISLNK (old_mode))
+      ch.old_mode = file_stats->st_mode;
+      ch.new_mode = mode_adjust (ch.old_mode, S_ISDIR (ch.old_mode) != 0,
+                                 umask_value, change, NULL);
+      if (chmodat (fts->fts_cwd_fd, file, ch.new_mode) == 0)
+        ch.status = CH_SUCCEEDED;
+      else
         {
-          if (chmodat (fts->fts_cwd_fd, file, new_mode) == 0)
-            chmod_succeeded = true;
-          else
-            {
-              if (! force_silent)
-                error (0, errno, _("changing permissions of %s"),
-                       quoteaf (file_full_name));
-              ok = false;
-            }
+          if (! force_silent)
+            error (0, errno, _("changing permissions of %s"),
+                   quoteaf (file_full_name));
+          ch.status = CH_FAILED;
         }
     }
 
   if (verbosity != V_off)
     {
-      bool changed = (chmod_succeeded
-                      && mode_changed (fts->fts_cwd_fd, file, file_full_name,
-                                       old_mode, new_mode));
+      if (ch.status == CH_SUCCEEDED
+          && !mode_changed (fts->fts_cwd_fd, file, file_full_name,
+                            ch.old_mode, ch.new_mode))
+        ch.status = CH_NO_CHANGE_REQUESTED;
 
-      if (changed || verbosity == V_high)
-        {
-          enum Change_status ch_status =
-            (!ok ? CH_FAILED
-             : !chmod_succeeded ? CH_NOT_APPLIED
-             : !changed ? CH_NO_CHANGE_REQUESTED
-             : CH_SUCCEEDED);
-          describe_change (file_full_name, old_mode, new_mode, ch_status);
-        }
+      if (ch.status == CH_SUCCEEDED || verbosity == V_high)
+        describe_change (file_full_name, &ch);
     }
 
-  if (chmod_succeeded && diagnose_surprises)
+  if (CH_NO_CHANGE_REQUESTED <= ch.status && diagnose_surprises)
     {
       mode_t naively_expected_mode =
-        mode_adjust (old_mode, S_ISDIR (old_mode) != 0, 0, change, NULL);
-      if (new_mode & ~naively_expected_mode)
+        mode_adjust (ch.old_mode, S_ISDIR (ch.old_mode) != 0, 0, change, NULL);
+      if (ch.new_mode & ~naively_expected_mode)
         {
           char new_perms[12];
           char naively_expected_perms[12];
-          strmode (new_mode, new_perms);
+          strmode (ch.new_mode, new_perms);
           strmode (naively_expected_mode, naively_expected_perms);
           new_perms[10] = naively_expected_perms[10] = '\0';
           error (0, 0,
                  _("%s: new permissions are %s, not %s"),
                  quotef (file_full_name),
                  new_perms + 1, naively_expected_perms + 1);
-          ok = false;
+          ch.status = CH_FAILED;
         }
     }
 
   if ( ! recurse)
     fts_set (fts, ent, FTS_SKIP);
 
-  return ok;
+  return CH_NO_CHANGE_REQUESTED <= ch.status;
 }
 
 /* Recursively change the modes of the specified FILES (the last entry
-- 
2.30.2

Subject: chmod reads uninitialized variables when fts_info is an error and -v is set, leading to random error messages
From: Michael Debertol <[email-masked]>
To: [email-masked]
Date: Sun, 15 Aug 2021 22:15:58 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

I noticed that when running chmod with -v on a dangling symlink the 
error message is somewhat random:

 > ln -s file-that-does-not-exist lnk

 > chmod -w lnk -v

 > chmod: cannot operate on dangling symlink 'lnk'

 > failed to change mode of 'lnk' from 0000 (---------) to 7777 (rwsrwsrwt)

 > chmod -w lnk -v

 > chmod: cannot operate on dangling symlink 'lnk'

 > failed to change mode of 'lnk' from 0000 (---------) to 7775 (rwsrwsr-t)

 > chmod -w lnk -v

 > chmod: cannot operate on dangling symlink 'lnk'

 > failed to change mode of 'lnk' from 0000 (---------) to 7774 (rwsrwsr-T)

(note that the value for "to" is different)

This appears to be because in the process_file function, old_mode and 
new_mode are only set when ok is true. When chmod encounters a dangling 
symlink, ok will be false and old_mode/new_mode won't be set. However, 
old_mode and new_mode are still accessed for the invocation of 
describe_change if verbosity is set to high, leading to incorrect error 
messages.

I think a better behavior could be to not print the "from mode to mode" 
part of the error message if the file could not be accessed, since it is 
meaningless anyways.

Have a nice day,

Michael