Hi Pádraig, Good point, thanks. v2 leaves setdefaultfilecon() inside change_attributes(). Updated patch attached.
I began to wonder if need_copy() should also do a more rigorous check for the "selinux_enabled && use_default_selinux_context" case, to see if the destination already has the default context. But that's slightly out of scope of this patch, so just FYI. Thanks, Mateusz On Wed, Jul 15, 2026 at 1:18 PM Pádraig Brady <[email protected]> wrote: > > On 14/07/2026 23:30, Mateusz Nosek wrote: > > Hi, > > > > I attach a patch for the "install" utility: "install -C" was still > > calling chown/chmod on the destination even when need_copy() had > > already determined it matches (mode, owner, group, content), which > > could needlessly fail, e.g. on an immutable file. > > > > This is my first patch to coreutils, so apologies for any mistakes in > > the submission process -- happy to fix anything you point out. > This makes sense I think. > > You moved setdefaultfilecon() out of this consideration. > I'd be inclined to leave it where it was and treat the same way, > because the comparison also compares selinux context, > and also if the file is already present it probably has an appropriate > context anyway. > > thanks, > Padraig
From de1209ad0644523c4150aa04103565a0f3241466 Mon Sep 17 00:00:00 2001 From: Mateusz Nosek <[email protected]> Date: Tue, 14 Jul 2026 23:50:28 +0200 Subject: [PATCH v2] install: skip chown/chmod when -C finds nothing to change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "install -C" (--compare) has always called chown/chmod on the destination even after determining that copying was unnecessary, ever since --compare was added in commit v7.0-182-gdac5f12c6. need_copy() already verifies that the destination's mode, owner, and group match (and, with --preserve-context, that the SELinux context matches too) before skipping the copy, so re-applying them is redundant, and can needlessly fail on a destination that cannot be chown/chmod'd even though nothing would actually change, e.g. one marked immutable with chattr +i. [bug introduced in coreutils-7.1] * src/install.c (install_file_in_file): Skip change_attributes when the copy was skipped by --compare (-C), i.e. copy_status == COPY_SKIPPED, since need_copy already verified matching mode, owner, and group in that case. * tests/install/install-C-immutable.sh: New test. * tests/local.mk (all_root_tests): Add it. Suggested by Pádraig Brady. --- src/install.c | 4 +- tests/install/install-C-immutable.sh | 58 ++++++++++++++++++++++++++++ tests/local.mk | 1 + 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100755 tests/install/install-C-immutable.sh diff --git a/src/install.c b/src/install.c index bccfe606f..1eae1a5ff 100644 --- a/src/install.c +++ b/src/install.c @@ -737,7 +737,9 @@ install_file_in_file (char const *from, char const *to, || ! S_ISREG (from_sb.st_mode)) && ! change_timestamps (&from_sb, to, to_dirfd, to_relname)) return false; - return change_attributes (to, to_dirfd, to_relname); + + return copy_status == COPY_SKIPPED + || change_attributes (to, to_dirfd, to_relname); } /* Create any missing parent directories of TO, diff --git a/tests/install/install-C-immutable.sh b/tests/install/install-C-immutable.sh new file mode 100755 index 000000000..3b0cbf4d2 --- /dev/null +++ b/tests/install/install-C-immutable.sh @@ -0,0 +1,58 @@ +#!/bin/sh +# Ensure "install -C" does not attempt to chown/chmod an unchanged, +# immutable destination file. +# Requires root access to do chattr +i, as well as a file system +# that supports the immutable attribute (e.g., ext2/3/4, xfs, btrfs). + +# 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_ ginstall +require_root_ + +# These simple one-file operations are expected to work even in the +# presence of this bug, and we need them to set up the rest of the test. +chattr_i_works=1 +touch f || framework_failure_ +chattr +i f 2>/dev/null || chattr_i_works=0 +rm f 2>/dev/null +test -f f || chattr_i_works=0 +chattr -i f 2>/dev/null || chattr_i_works=0 +rm f 2>/dev/null || chattr_i_works=0 +test -f f && chattr_i_works=0 + +if test $chattr_i_works = 0; then + skip_ "chattr +i doesn't work on this file system" +fi + +u1=1 +g1=1 + +echo test > a || framework_failure_ + +# Install once to create the destination, then make it immutable. +ginstall -Cv -m0644 -o$u1 -g$g1 a b || framework_failure_ +chattr +i b || framework_failure_ + +# A second "-C" install with an identical source, mode, owner and group +# must be a no-op: it must not attempt to chown/chmod the destination, +# since that would fail on an immutable file even though nothing about +# it would actually change. +ginstall -Cv -m0644 -o$u1 -g$g1 a b || fail=1 + +chattr -i b + +Exit $fail diff --git a/tests/local.mk b/tests/local.mk index e2afbe134..18bf79861 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -121,6 +121,7 @@ all_root_tests = \ tests/du/bind-mount-dir-cycle.sh \ tests/du/bind-mount-dir-cycle-v2.sh \ tests/id/setgid.sh \ + tests/install/install-C-immutable.sh \ tests/install/install-C-root.sh \ tests/ls/capability.sh \ tests/ls/no-cap.sh \ -- 2.47.3
