>From af4711d5dc15f0c014bfd9c92f4eadedb89f732f Mon Sep 17 00:00:00 2001 From: Paul Eggert 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