bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

"chmod -w file" now complains if file is still writable afterwards


From: Paul Eggert
Subject: "chmod -w file" now complains if file is still writable afterwards
Date: Wed, 04 May 2005 10:30:24 -0700
User-agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.4 (gnu/linux)

For quite some time I've been annoyed that "chmod -w file" can leave
the file writeable afterwards, if your umask is restrictive.  You're
supposed to use "chmod a-w file" if you really want the file to be
unwriteable.

This is a common trap for novices to fall into, and I think it'd be
better if chmod diagnosed the mistake, in addition to performing the
requested action.  I just checked POSIX, and it allows "chmod" to
diagnose errors like "chmod -w file" so I installed the following
patch.

Another option would be for "chmod -w file" to silently adjust itself
to behave like "chmod a-w file"; POSIX allows that too.  If you think
that's better I could install a patch along those lines instead.

2005-05-04  Paul Eggert  <address@hidden>

        * NEWS: chmod -w now complains if it differs from chmod a-w.
        * src/chmod.c: Include quotearg.h.
        (diagnose_surprises): New var.
        (process_file): Diagnose surprises.  Simplify the logic a bit,
        while we're at it.
        (main): Prepare to diagnose surprises.  Remove useless code for
        '-' option.
        * tests/chmod/Makefile.am (TESTS): Add umask-x.
        * tests/chmod/umask-x: New file.

Index: NEWS
===================================================================
RCS file: /fetish/cu/NEWS,v
retrieving revision 1.284
diff -p -u -r1.284 NEWS
--- NEWS        2 May 2005 18:40:20 -0000       1.284
+++ NEWS        4 May 2005 17:18:47 -0000
@@ -124,6 +124,9 @@ GNU coreutils NEWS                      
 
 ** New features
 
+  chmod -w now complains if its behavior differs from what chmod a-w
+  would do, and similarly for chmod -r, chmod -x, etc.
+
   join now supports a NUL field separator, e.g., "join -t '\0'".
   join now detects and reports incompatible options, e.g., "join -t x -t y",
 
Index: doc/coreutils.texi
===================================================================
RCS file: /fetish/cu/doc/coreutils.texi,v
retrieving revision 1.254
diff -p -u -r1.254 coreutils.texi
--- doc/coreutils.texi  2 May 2005 18:41:12 -0000       1.254
+++ doc/coreutils.texi  4 May 2005 17:18:48 -0000
@@ -8451,9 +8451,11 @@ recursive directory traversals.
 
 If used, @var{mode} specifies the new permissions.
 For details, see the section on @ref{File permissions}.
-In the extremely rare cases where @var{mode} has leading @samp{-}, a
-portable script should use @option{--} first, e.g., @samp{chmod -- -w
-file}.  Typically, though, @samp{chmod a-w file} is preferable.
+If you really want @var{mode} to have a leading @samp{-}, you should
+use @option{--} first, e.g., @samp{chmod -- -w file}.  Typically,
+though, @samp{chmod a-w file} is preferable, and @command{chmod -w
+file} (without the @option{--}) complains if it behaves differently
+from what @samp{chmod a-w file} would do.
 
 The program accepts the following options.  Also see @ref{Common options}.
 
Index: src/chmod.c
===================================================================
RCS file: /fetish/cu/src/chmod.c,v
retrieving revision 1.109
diff -p -u -r1.109 chmod.c
--- src/chmod.c 28 Apr 2005 16:29:59 -0000      1.109
+++ src/chmod.c 4 May 2005 17:18:48 -0000
@@ -29,6 +29,7 @@
 #include "filemode.h"
 #include "modechange.h"
 #include "quote.h"
+#include "quotearg.h"
 #include "root-dev-ino.h"
 #include "xfts.h"
 
@@ -72,6 +73,11 @@ static bool recurse;
 /* If true, force silence (no error messages). */
 static bool force_silent;
 
+/* If true, diagnose surprises from naive misuses like "chmod -r file".
+   POSIX allows diagnostics here, as portable code is supposed to use
+   "chmod -- -r file".  */
+static bool diagnose_surprises;
+
 /* Level of verbosity.  */
 static enum Verbosity verbosity = V_off;
 
@@ -176,10 +182,10 @@ 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 do_chmod;
-  bool symlink_changed = true;
+  bool chmod_succeeded = false;
 
   switch (ent->fts_info)
     {
@@ -206,48 +212,65 @@ process_file (FTS *fts, FTSENT *ent)
       break;
     }
 
-  do_chmod = ok;
-
-  if (do_chmod && ROOT_DEV_INO_CHECK (root_dev_ino, file_stats))
+  if (ok && ROOT_DEV_INO_CHECK (root_dev_ino, file_stats))
     {
       ROOT_DEV_INO_WARN (file_full_name);
       ok = false;
-      do_chmod = false;
     }
 
-  if (do_chmod)
+  if (ok)
     {
-      new_mode = mode_adjust (file_stats->st_mode, change, umask_value);
+      old_mode = file_stats->st_mode;
+      new_mode = mode_adjust (old_mode, change, umask_value);
 
-      if (S_ISLNK (file_stats->st_mode))
-       symlink_changed = false;
-      else
+      if (! S_ISLNK (old_mode))
        {
-         ok = (chmod (file, new_mode) == 0);
-
-         if (! (ok | force_silent))
-           error (0, errno, _("changing permissions of %s"),
-                  quote (file_full_name));
+         if (chmod (file, new_mode) == 0)
+           chmod_succeeded = true;
+         else
+           {
+             if (! force_silent)
+               error (0, errno, _("changing permissions of %s"),
+                      quote (file_full_name));
+             ok = false;
+           }
        }
     }
 
   if (verbosity != V_off)
     {
-      bool changed =
-       (ok && symlink_changed
-        && mode_changed (file, file_stats->st_mode, new_mode));
+      bool changed = (chmod_succeeded
+                     && mode_changed (file, old_mode, new_mode));
 
       if (changed || verbosity == V_high)
        {
          enum Change_status ch_status =
            (!ok ? CH_FAILED
-            : !symlink_changed ? CH_NOT_APPLIED
+            : !chmod_succeeded ? CH_NOT_APPLIED
             : !changed ? CH_NO_CHANGE_REQUESTED
             : CH_SUCCEEDED);
          describe_change (file_full_name, new_mode, ch_status);
        }
     }
 
+  if (chmod_succeeded & diagnose_surprises)
+    {
+      mode_t naively_expected_mode = mode_adjust (old_mode, change, 0);
+      if (new_mode & ~naively_expected_mode)
+       {
+         char new_perms[11];
+         char naively_expected_perms[11];
+         mode_string (new_mode, new_perms);
+         mode_string (naively_expected_mode, naively_expected_perms);
+         new_perms[10] = naively_expected_perms[10] = '\0';
+         error (0, 0,
+                _("%s: new permissions are %s, not %s"),
+                quotearg_colon (file_full_name),
+                new_perms + 1, naively_expected_perms + 1);
+         ok = false;
+       }
+    }
+
   if ( ! recurse)
     fts_set (fts, ent, FTS_SKIP);
 
@@ -354,10 +377,10 @@ main (int argc, char **argv)
 
   atexit (close_stdout);
 
-  recurse = force_silent = false;
+  recurse = force_silent = diagnose_surprises = false;
 
   while ((c = getopt_long (argc, argv,
-                          "Rcfvr::w::x::X::s::t::u::g::o::a::,::+::-::=::",
+                          "Rcfvr::w::x::X::s::t::u::g::o::a::,::+::=::",
                           long_options, NULL))
         != -1)
     {
@@ -375,8 +398,11 @@ main (int argc, char **argv)
        case 'a':
        case ',':
        case '+':
-       case '-':
        case '=':
+         /* Support nonportable uses like "chmod -w", but diagnose
+            surprises due to umask confusion.  Even though "--", "--r",
+            etc., are valid modes, there is no "case '-'" here since
+            getopt_long reserves leading "--" for long options.  */
          {
            /* Allocate a mode string (e.g., "-rwx") by concatenating
               the argument containing this option.  If a previous mode
@@ -395,6 +421,8 @@ main (int argc, char **argv)
            mode[mode_len] = ',';
            strcpy (mode + mode_comma_len, arg);
            mode_len = new_mode_len;
+
+           diagnose_surprises = true;
          }
          break;
        case NO_PRESERVE_ROOT:
Index: tests/chmod/Makefile.am
===================================================================
RCS file: /fetish/cu/tests/chmod/Makefile.am,v
retrieving revision 1.8
diff -p -u -r1.8 Makefile.am
--- tests/chmod/Makefile.am     24 Sep 2004 23:34:41 -0000      1.8
+++ tests/chmod/Makefile.am     4 May 2005 17:18:48 -0000
@@ -1,7 +1,7 @@
 ## Process this file with automake to produce Makefile.in -*-Makefile-*-.
 AUTOMAKE_OPTIONS = 1.4 gnits
 
-TESTS = no-x equals equal-x c-option setgid usage
+TESTS = no-x equals equal-x c-option setgid umask-x usage
 EXTRA_DIST = $(TESTS)
 TESTS_ENVIRONMENT = \
   PATH="`pwd`/../../src$(PATH_SEPARATOR)$$PATH"
--- /dev/null   2003-03-18 13:55:57 -0800
+++ tests/chmod/umask-x 2005-05-04 10:12:38 -0700
@@ -0,0 +1,36 @@
+#!/bin/sh
+# Test that chmod -x file reports an error if the result is executable.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  chmod --version
+fi
+
+. $srcdir/../lang-default
+
+pwd=`pwd`
+tmp=minus.$$
+trap 'status=$?; cd $pwd; rm -rf $tmp && exit $status' 0
+trap '(exit $?); exit' 1 2 13 15
+
+framework_failure=0
+mkdir $tmp || framework_failure=1
+cd $tmp || framework_failure=1
+
+if test $framework_failure = 1; then
+  echo "$0: failure in testing framework" 1>&2
+  (exit 1); exit 1
+fi
+
+fail=0
+
+touch file
+chmod 755 file
+(umask 77 && chmod -x file) 2>/dev/null && fail=1
+
+if test $framework_failure = 1; then
+  echo "$0: failure in testing framework" 1>&2
+  fail=1
+fi
+
+(exit $fail); exit $fail




reply via email to

[Prev in Thread] Current Thread [Next in Thread]