GNU bug report logs - #17035
[PATCH] chmod -c -R produces errors with special permissions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 17035 in the body.
You can then email your comments to 17035 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dylan Alex Simon <dylan <at> dylex.net>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] chmod -c -R produces errors with special permissions
Date: Tue, 18 Mar 2014 12:06:31 -0400

[Message part 1 (text/plain, inline)]

chmod sometimes tries to stat files in the wrong directory when reporting
changes:

> mkdir -p a/b
> chmod -R g+s a/b # requires some special permission bit set first
> chmod -c -R g+w a
chmod: getting new attributes of 'b': No such file or directory

A fix is attached.

[0001-chmod-fix-mode_changed-check.patch (text/plain, attachment)]

Message #10 received at 17035-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Dylan Alex Simon <dylan <at> dylex.net>
Cc: 17035-done <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 18:45:29 +0000

On 03/18/2014 04:06 PM, Dylan Alex Simon wrote:
> chmod sometimes tries to stat files in the wrong directory when reporting
> changes:
> 
>> mkdir -p a/b
>> chmod -R g+s a/b # requires some special permission bit set first
>> chmod -c -R g+w a
> chmod: getting new attributes of 'b': No such file or directory
> 
> A fix is attached.
> 

The fix looks good.  It seems this bug was introduced
with the change to FTS in 5.1.0 (released Dec 2003)
I'll push soon in your name with these changes merged in.

thanks!
Pádraig.

$ git diff HEAD@{1}

diff --git a/NEWS b/NEWS
index 35d48e5..3623674 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-

 ** Bug fixes

+  chmod -R --changes no longer issues erroneous warnings for files with special
+  bits set.  [bug introduced in coreutils-5.1.0]
+
   cp -a, mv, and install --preserve-context, once again set the correct SELinux
   context for existing directories in the destination.  Previously they set
   the context of an existing directory to that of its last copied descendent.
diff --git a/src/chmod.c b/src/chmod.c
index f9debde..ae8b6fb 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -111,7 +111,7 @@ static struct option const long_options[] =
    The old mode was OLD_MODE, but it was changed to NEW_MODE.  */

 static bool
-mode_changed (int dirfd, char const *file, char const *file_full_name,
+mode_changed (int dir_fd, char const *file, char const *file_full_name,
               mode_t old_mode, mode_t new_mode)
 {
   if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
@@ -121,7 +121,7 @@ mode_changed (int dirfd, char const *file, char const *file_full_name,

       struct stat new_stats;

-      if (fstatat (dirfd, file, &new_stats, 0) != 0)
+      if (fstatat (dir_fd, file, &new_stats, 0) != 0)
         {
           if (! force_silent)
             error (0, errno, _("getting new attributes of %s"), quote (file_full_name));
diff --git a/tests/chmod/c-option.sh b/tests/chmod/c-option.sh
index 1dd9b9e..38b127b 100755
--- a/tests/chmod/c-option.sh
+++ b/tests/chmod/c-option.sh
@@ -37,4 +37,15 @@ case "$(cat out)" in
   *) cat out; fail=1 ;;
 esac

+# From V5.1.0 to 8.22 this would stat the wrong file and
+# give an erroneous ENOENT diagnostic
+mkdir -p a/b || framework_failure_
+# chmod g+s might fail as detailed in setgid.sh
+# but we don't care about those edge cases here
+chmod g+s a/b
+# This should never warn, but in did when special
+# bits are set on b (the common case under test)
+chmod -c -R g+w a 2>err
+test -s err && fail=1
+
 Exit $fail

Message #13 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 17035 <at> debbugs.gnu.org, P <at> draigBrady.com, dylan <at> dylex.net
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 23:18:25 +0100

On 03/18/2014 07:45 PM, Pádraig Brady wrote:
> The fix looks good.  It seems this bug was introduced
> with the change to FTS in 5.1.0 (released Dec 2003)

Wasn't it commit v5.2.1-690-gbc580f6?

> I'll push soon in your name with these changes merged in.

> $ git diff HEAD@{1}
> 
> diff --git a/NEWS b/NEWS
> index 35d48e5..3623674 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ GNU coreutils NEWS                                    -*- outline -*-
> 
>  ** Bug fixes
> 
> +  chmod -R --changes no longer issues erroneous warnings for files with special
> +  bits set.  [bug introduced in coreutils-5.1.0]
> +

v5.3.0? See above.

>    cp -a, mv, and install --preserve-context, once again set the correct SELinux
>    context for existing directories in the destination.  Previously they set
>    the context of an existing directory to that of its last copied descendent.
> diff --git a/src/chmod.c b/src/chmod.c
> index f9debde..ae8b6fb 100644
> --- a/src/chmod.c
> +++ b/src/chmod.c
> @@ -111,7 +111,7 @@ static struct option const long_options[] =
>     The old mode was OLD_MODE, but it was changed to NEW_MODE.  */
> 
>  static bool
> -mode_changed (int dirfd, char const *file, char const *file_full_name,
> +mode_changed (int dir_fd, char const *file, char const *file_full_name,
>                mode_t old_mode, mode_t new_mode)
>  {
>    if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
> @@ -121,7 +121,7 @@ mode_changed (int dirfd, char const *file, char const *file_full_name,
> 
>        struct stat new_stats;
> 
> -      if (fstatat (dirfd, file, &new_stats, 0) != 0)
> +      if (fstatat (dir_fd, file, &new_stats, 0) != 0)
>          {
>            if (! force_silent)
>              error (0, errno, _("getting new attributes of %s"), quote (file_full_name));
> diff --git a/tests/chmod/c-option.sh b/tests/chmod/c-option.sh
> index 1dd9b9e..38b127b 100755
> --- a/tests/chmod/c-option.sh
> +++ b/tests/chmod/c-option.sh
> @@ -37,4 +37,15 @@ case "$(cat out)" in
>    *) cat out; fail=1 ;;
>  esac
> 
> +# From V5.1.0 to 8.22 this would stat the wrong file and
> +# give an erroneous ENOENT diagnostic
> +mkdir -p a/b || framework_failure_
> +# chmod g+s might fail as detailed in setgid.sh
> +# but we don't care about those edge cases here
> +chmod g+s a/b
> +# This should never warn, but in did when special

s/ in / it /

> +# bits are set on b (the common case under test)
> +chmod -c -R g+w a 2>err
> +test -s err && fail=1
> +
>  Exit $fail

Thanks.

One question: I did not dig into this deeper yet, but what exactly
is the connection between "directory with special permissions"
vs. "stat()ing the wrong file"?
I'm asking because incidentally yesterday I saw the same warning
from "chmod -R -c" when playing with recursive bind-mounts, i.e.,
there were no files or directories with special bits set.

Thanks & have a nice day,
Berny

Message #16 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 17035 <at> debbugs.gnu.org, P <at> draigBrady.com, dylan <at> dylex.net
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 23:33:39 +0100

On 03/18/2014 07:45 PM, Pádraig Brady wrote:
> @@ -121,7 +121,7 @@ mode_changed (int dirfd, char const *file, char const *file_full_name,
> 
>        struct stat new_stats;
> 
> -      if (fstatat (dirfd, file, &new_stats, 0) != 0)
> +      if (fstatat (dir_fd, file, &new_stats, 0) != 0)
>          {
>            if (! force_silent)
>              error (0, errno, _("getting new attributes of %s"), quote (file_full_name));

Another minor nit: the last line doesn't pass sc_long_lines:

  $ make syntax-check
  ...
  src/chmod.c:127:            error (0, errno, _("getting new attributes of %s"), quote (file_full_name));
  maint.mk: line(s) with more than 80 characters; reindent

Thanks & have a nice day,
Berny

Message #19 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 17035 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>,
 dylan <at> dylex.net
Cc: 17035-done <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 16:36:37 -0700

On Tue, Mar 18, 2014 at 11:45 AM, Pádraig Brady <P <at> draigbrady.com> wrote:
> diff --git a/tests/chmod/c-option.sh b/tests/chmod/c-option.sh
...
> +# This should never warn, but in did when special
> +# bits are set on b (the common case under test)
> +chmod -c -R g+w a 2>err
> +test -s err && fail=1

Thanks for adding the test.
I have a small preference for using compare-vs-/dev/null rather than
"test -s," since the former will print the unexpected output in the log,
while the latter won't. So maybe this instead?

  compare /dev/null err || fail=1

Message #25 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: dylan <at> dylex.net, 17035 <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 23:41:19 +0000

On 03/18/2014 10:18 PM, Bernhard Voelker wrote:
> On 03/18/2014 07:45 PM, Pádraig Brady wrote:
>> The fix looks good.  It seems this bug was introduced
>> with the change to FTS in 5.1.0 (released Dec 2003)
> 
> Wasn't it commit v5.2.1-690-gbc580f6?

I think that restricted the issue to if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
but the stat on the relative name was there previously?

> One question: I did not dig into this deeper yet, but what exactly
> is the connection between "directory with special permissions"
> vs. "stat()ing the wrong file"?
> I'm asking because incidentally yesterday I saw the same warning
> from "chmod -R -c" when playing with recursive bind-mounts, i.e.,
> there were no files or directories with special bits set.

Interesting. Looking at the code seems that message
is restricted to the above condition where those bits are set.
Can you reproduce?

thanks,
Pádraig.

Message #28 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: dylan <at> dylex.net, 17035 <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Tue, 18 Mar 2014 23:40:11 +0000

On 03/18/2014 11:36 PM, Jim Meyering wrote:
> compare /dev/null err || fail=1

Yes better thanks

Message #31 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: dylan <at> dylex.net, 17035 <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Wed, 19 Mar 2014 02:38:54 +0000

On 03/18/2014 11:41 PM, Pádraig Brady wrote:
> On 03/18/2014 10:18 PM, Bernhard Voelker wrote:
>> On 03/18/2014 07:45 PM, Pádraig Brady wrote:
>>> The fix looks good.  It seems this bug was introduced
>>> with the change to FTS in 5.1.0 (released Dec 2003)
>>
>> Wasn't it commit v5.2.1-690-gbc580f6?
> 
> I think that restricted the issue to if (new_mode & (S_ISUID | S_ISGID | S_ISVTX))
> but the stat on the relative name was there previously?

But if that was the case then previous to v5.2.1 release
would have gotten the diagnostic without special mode bits,
which is unlikely as the issue would then very likely have been noticed.
So I dug a little further and the issue seems to be with where fts
was improved to not change directory when traversing in coreutils 6.0 in:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=c1994c16

Change is now pushed:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=09eda9ed

thanks,
Pádraig.

Message #34 received at 17035 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: dylan <at> dylex.net, 17035 <at> debbugs.gnu.org
Subject: Re: bug#17035: [PATCH] chmod -c -R produces errors with special
 permissions
Date: Wed, 19 Mar 2014 08:57:09 +0100

On 03/19/2014 12:41 AM, Pádraig Brady wrote:
> On 03/18/2014 10:18 PM, Bernhard Voelker wrote:
>> One question: I did not dig into this deeper yet, but what exactly
>> is the connection between "directory with special permissions"
>> vs. "stat()ing the wrong file"?
>> I'm asking because incidentally yesterday I saw the same warning
>> from "chmod -R -c" when playing with recursive bind-mounts, i.e.,
>> there were no files or directories with special bits set.
>
> Interesting. Looking at the code seems that message
> is restricted to the above condition where those bits are set.
> Can you reproduce?

Yes, fortunately - and now I see that my case implicitly was
using +t mode because of the TMPFS file system type:

  $ mount -t tmpfs /mnt /mnt

  $ mkdir -p /mnt/dir/mnt

  $ mount --bind /mnt /mnt/dir/mnt

  $ cd /mnt

  $ ln -s dir/mnt/dir d

  $ ls -ldogi . d dir dir/mnt
  186326 drwxrwxrwt 3 80 Mar 19 08:31 .
  188029 lrwxrwxrwx 1 11 Mar 19 08:31 d -> dir/mnt/dir
  188511 drwxr-xrwx 3 60 Mar 19 08:31 dir
  186326 drwxrwxrwt 3 80 Mar 19 08:31 dir/mnt

  $ chmod -Rc o+w dir
  mode of ‘dir’ changed from 0755 (rwxr-xr-x) to 0757 (rwxr-xrwx)
  chmod: getting new attributes of ‘mnt’: No such file or directory
  mode of ‘dir/mnt/dir/mnt’ changed from 0755 (rwxr-xr-x) to 0757 (rwxr-xrwx)

As a final side note to this case, I want to mention that the issue
also happens with the -v option, of course:

  $ mkdir -p a/b
  $ chmod +t a/b
  $ chmod -Rv o+w a
  mode of ‘a’ changed from 0755 (rwxr-xr-x) to 0757 (rwxr-xrwx)
  chmod: getting new attributes of ‘b’: No such file or directory
  mode of ‘a/b’ retained as 1757 (rwxr-xrwt)

Thanks & have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.