Pádraig Brady <[email protected]> writes:
> On 20/02/2026 03:16, Collin Funk wrote:
>> It probably isn't super clear what this test case is for, so here is a
>> more obvious example:
>> $ stat --format=%U /
>> root
>> $ id -un
>> collin
>> $ chmod = /
>> chmod: changing permissions of '/': Operation not permitted
>
> For ref that was previously discussed at:
> http://bugs.debian.org/497514
>
> We don't make this a noop as there can be ACL clearing side effects.
> Also there are TOCTOU issues with doing this in userspace.
The TOCTOU issue was also something I was thinking about.
My preference is the current behavior, and I doubt we will want to
change the longstanding behavior with respect to chown() side-effects,
so it might be worth removing that TODO entry.
But I do think that reasonable minds can disagree here, and I don't see
any wording in POSIX that disallows skipping the chown() call.
>> If one tries to skip calling chmod(2) because the file mode bits are
>> unchanged, they will get the incorrect behavior.
>> That obviously wouldn't work when the tests are run as root, so I
>> think that 'strace' is the best way to test this.
>
>> +print_ver_ chmod
>> +uses_strace_
>> +getlimits_
>> +
>> +echo 'hello' > file || framework_failure_
>> +
>> +cat <<EOF > exp || framework_failure_
>> +chmod: changing permissions of 'file': $EPERM
>> +EOF
>> +
>> +for op in '+' '-' '='; do
>> + returns_ 1 strace -o /dev/null -P file -e quiet=path-resolution \
>> + -e '/f?chmod' -e fault=all:error=EPERM chmod "$op" file 2>err \
>> + || fail=1
>> + compare exp err || fail=1
>> +done
>
> I don't think this would get EPERM on AIX, or if strace is not installed?
> I'm thinking not using strace and just using skip_if_root_
> would be safer anyway. We don't need this to run as root
> so it's fine to skip in that edge case.
>
> You could also: test "$(stat --format=%U /)" = 'root' || skip_ ...
> for extra safety.
Oh, I had meant to use 'require_strace_' instead of
'uses_strace_'. Sorry for causing some confusion.
The 'skip_if_root_' and extra safety check sounds like a good idea, so I
pushed the attached patch.
I just checked for an error message being emitted, not a specific error
string, since on the MacOS cfarm machine:
$ chmod = /
chmod: Unable to change file mode on /: Read-only file system
Though, GNU/Linux will do the same with a read-only root:
$ sudo mkdir -p /mnt/test-tmpfs
$ sudo mount -t tmpfs -o ro,size=1G tmpfs /mnt/test-tmpfs
$ chmod = /mnt/test-tmpfs
chmod: changing permissions of '/mnt/test-tmpfs': Read-only file system
Collin
>From d233ebd7d9e36208f5bc03935f910de4c3283dd1 Mon Sep 17 00:00:00 2001
Message-ID: <d233ebd7d9e36208f5bc03935f910de4c3283dd1.1771654829.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Thu, 19 Feb 2026 19:13:34 -0800
Subject: [PATCH v2] tests: chmod: test that chmod(2) is always called
* tests/chmod/only-op.sh: New file.
* tests/local.mk (all_test): Add the new test.
---
tests/chmod/only-op.sh | 31 +++++++++++++++++++++++++++++++
tests/local.mk | 1 +
2 files changed, 32 insertions(+)
create mode 100755 tests/chmod/only-op.sh
diff --git a/tests/chmod/only-op.sh b/tests/chmod/only-op.sh
new file mode 100755
index 000000000..a2fd2546a
--- /dev/null
+++ b/tests/chmod/only-op.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+# Test that 'chmod' does not skip calling chmod(2) even when the file mode bits
+# are unchanged.
+
+# Copyright (C) 2026 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ chmod stat
+skip_if_root_
+
+test "$(stat --format=%U /)" = 'root' || skip_ 'root does not own /'
+
+for op in '+' '-' '='; do
+ returns_ 1 chmod "$op" / 2>err || fail=1
+ grep -F "chmod: changing permissions of '/'" err || { fail=1; cat err; }
+done
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 5f9f3d19b..aa71951f8 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -517,6 +517,7 @@ all_tests = \
tests/chmod/ignore-symlink.sh \
tests/chmod/inaccessible.sh \
tests/chmod/octal.sh \
+ tests/chmod/only-op.sh \
tests/chmod/partial-fail.sh \
tests/chmod/setgid.sh \
tests/chmod/silent.sh \
--
2.53.0
