Pádraig Brady <[email protected]> writes:
> On 03/11/2026 05:08, Collin Funk wrote:
>> It would also be good to add a test for other races, but I still need
>> to think of the best way to do it. Perhaps for the rename one, a long
>> file name could maybe reproduce it consistently, since it calls
>> renameatu N times where N is the length of the file name.
>> The chmod one can probably only be done with a breakpoint or
>> something
>> like that.
>
> I was thinking about using strace to send a SIGSTOP for something similar
> recently,
> so it's probably worth adding such a test for reference at least.
> I've attached a test that fails without the fix and passes with it.
Nice! I'm gradually learning that I know very little of the features
that 'strace' has. That is a useful one.
Feel free to commit that. I didn't want to take your authorship by
adding it to my commit.
>> diff --git a/src/shred.c b/src/shred.c
>
>> + ok = (do_wipefd (fd, qname, s, flags)
>> + && (! flags->remove_file || wipename (dirfd, name, qname, flags)));
>> - if (close (fd) != 0)
>> + done:
>> + if (0 <= fd && close (fd) < 0)
>> {
>> error (0, errno, _("%s: failed to close"), qname);
>> ok = false;
>> }
>> - if (ok && flags->remove_file)
>> - ok = wipename (name, qname, flags);
>> + if (dirfd != AT_FDCWD)
>> + close (dirfd);
>> return ok;
>> }
> That unlinks the file even if close() returned EIO.
Oops, I was trying to keep that function reasonably clean with respect
to cleaning things up. Now I see that most of that function can be left
as-is to preserve that behavior. I pushed the attached 0001 patch, which
is the same as the earlier one, but with that behavior unchanged.
> I think it would be better to reorder to keep the file around in that case,
> at least as a witness to the failing overwrite operation.
Yes, good point. I pushed the attached 0002 patch adding a test for that
by injecting a close failure with 'strace'. The rationale wasn't
immediately obvious to me, so that should hopefully prevent me from
breaking it in the future.
Thanks for the review,
Collin
>From bfceb456371d219117e63702e149a9bdc992ea55 Mon Sep 17 00:00:00 2001
Message-ID: <bfceb456371d219117e63702e149a9bdc992ea55.1793767111.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Mon, 2 Nov 2026 21:08:13 -0800
Subject: [PATCH 1/2] shred: fix races with --force and --remove
* src/shred.c (wipefile): When --force or --remove are in use, and the
file is in a subdirectory, open a file descriptor to its parent and
perform operations on the basename relative to it.
(wipename): Add a file descriptor argument to the parent directory. Open
a readable file descriptor to the directory if we need to sync it. Use
the file descriptor with renameatu and unlinkat.
* tests/shred/unlink-race.sh: New file.
* tests/local.mk (all_tests): Add the new test case.
* NEWS: Mention the bug fixes.
Discovered by Anthropic's OSS Scanner, as bug ANT-2026-O4Q4UVP2.
---
NEWS | 11 ++++++
src/shred.c | 69 ++++++++++++++++++++++++++++----------
tests/local.mk | 1 +
tests/shred/unlink-race.sh | 59 ++++++++++++++++++++++++++++++++
4 files changed, 123 insertions(+), 17 deletions(-)
create mode 100755 tests/shred/unlink-race.sh
diff --git a/NEWS b/NEWS
index 2ae5b8301..b1d8f09ec 100644
--- a/NEWS
+++ b/NEWS
@@ -141,6 +141,17 @@ GNU coreutils NEWS -*- outline -*-
Also negative operands like "-,5" are no longer treated as options.
[bugs introduced in coreutils-8.24 and coreutils-8.31]
+ 'shred --force' no longer has races when opening non-writable files.
+ Previously, upon failing to open a writable file descriptor, it would
+ change its permissions before reopening it, following symbolic links in the
+ leading path components each time.
+ [This bug was present in "the beginning".]
+
+ 'shred --remove' no longer has races when renaming or unlinking files.
+ Previously, symbolic links in the leading path components were followed each
+ time the file was renamed and/or unlinked.
+ [This bug was present in "the beginning".]
+
'sort --compress-program=PROG' no longer treats an unset PATH environment
variable as equivalent to a PATH environment variable defined to an empty
string. On typical platforms, that would result in PROG being executed from
diff --git a/src/shred.c b/src/shred.c
index d96fefc78..6e7c4e4ab 100644
--- a/src/shred.c
+++ b/src/shred.c
@@ -1058,18 +1058,21 @@ incname (char *name, size_t len)
* file system with synchronous metadata updates, this is unnecessary.
*/
static bool
-wipename (char *oldname, char const *qoldname, struct Options const *flags)
+wipename (int const dirfd, char *oldname, char const *qoldname,
+ struct Options const *flags)
{
char *newname = xstrdup (oldname);
char *base = last_component (newname);
+ char *oldbase = oldname + (base - newname);
char *dir = dir_name (newname);
char *qdir = xstrdup (quotef (dir));
bool first = true;
bool ok = true;
- int dir_fd = -1;
+ int readable_dirfd = -1;
if (flags->remove_file == remove_wipesync)
- dir_fd = open (dir, O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
+ readable_dirfd = openat (dirfd, ".",
+ O_RDONLY | O_DIRECTORY | O_NOCTTY | O_NONBLOCK);
if (flags->verbose)
error (0, 0, _("%s: removing"), qoldname);
@@ -1080,14 +1083,14 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
memset (base, nameset[0], len);
base[len] = 0;
bool rename_ok;
- while (! (rename_ok = (renameatu (AT_FDCWD, oldname, AT_FDCWD, newname,
+ while (! (rename_ok = (renameatu (dirfd, oldbase, dirfd, base,
RENAME_NOREPLACE)
== 0))
&& errno == EEXIST && incname (base, len))
continue;
if (rename_ok)
{
- if (0 <= dir_fd && dosync (dir_fd, qdir) != 0)
+ if (0 <= readable_dirfd && dosync (readable_dirfd, qdir) != 0)
ok = false;
if (flags->verbose)
{
@@ -1100,22 +1103,22 @@ wipename (char *oldname, char const *qoldname, struct Options const *flags)
_("%s: renamed to %s"), old, newname);
first = false;
}
- memcpy (oldname + (base - newname), base, len + 1);
+ memcpy (oldbase, base, len + 1);
}
}
- if (unlink (oldname) != 0)
+ if (unlinkat (dirfd, oldbase, 0) < 0)
{
error (0, errno, _("%s: failed to remove"), qoldname);
ok = false;
}
else if (flags->verbose)
error (0, 0, _("%s: removed"), qoldname);
- if (0 <= dir_fd)
+ if (0 <= readable_dirfd)
{
- if (dosync (dir_fd, qdir) != 0)
+ if (dosync (readable_dirfd, qdir) != 0)
ok = false;
- if (close (dir_fd) != 0)
+ if (close (readable_dirfd) < 0)
{
error (0, errno, _("%s: failed to close"), qdir);
ok = false;
@@ -1143,11 +1146,38 @@ static bool
wipefile (char *name, char const *qname,
struct randint_source *s, struct Options const *flags)
{
- int fd = open (name, O_WRONLY | O_NOCTTY | O_NONBLOCK | O_BINARY);
+
+ int dirfd = AT_FDCWD;
+ char const *relname = name;
+ char const *base;
+
+ /* If we are using --force, we may need to change the permissions of NAME.
+ If we are using --remove, we will need to unlink or rename NAME.
+ In both cases, avoid following leading symbolic links, which may change
+ as 'shred' is executing. */
+ if ((flags->force || flags->remove_file)
+ && (base = last_component (name)) != name && *base)
+ {
+ char *dir = dir_name (name);
+
+ dirfd = open (dir, O_PATHSEARCH | O_DIRECTORY);
+ free (dir);
+
+ if (dirfd < 0)
+ {
+ error (0, errno, _("%s: failed to open for writing"), qname);
+ return false;
+ }
+
+ relname = base;
+ }
+
+ bool ok = true;
+ int fd = openat (dirfd, relname, O_WRONLY | O_NOCTTY | O_NONBLOCK | O_BINARY);
if (fd < 0
&& (errno == EACCES && flags->force)
- && chmod (name, S_IWUSR) == 0)
- fd = open (name, O_WRONLY | O_NOCTTY | O_NONBLOCK | O_BINARY);
+ && fchmodat (dirfd, relname, S_IWUSR, 0) == 0)
+ fd = openat (dirfd, relname, O_WRONLY | O_NOCTTY | O_NONBLOCK | O_BINARY);
if (fd < 0)
{
struct stat st;
@@ -1155,18 +1185,19 @@ wipefile (char *name, char const *qname,
/* Try to give a more meaningful error message if we attempt to
open a FIFO with no readers. */
- if (open_errno == ENXIO && 0 <= stat (name, &st)
+ if (open_errno == ENXIO && 0 <= fstatat (dirfd, relname, &st, 0)
&& S_ISFIFO (st.st_mode))
error (0, 0, _("%s: invalid file type"), qname);
else
error (0, open_errno, _("%s: failed to open for writing"), qname);
- return false;
+ ok = false;
+ goto done;
}
/* We used O_NONBLOCK to prevent blocking when opening a FIFO.
Reset that here. */
int fd_flags = fcntl (fd, F_GETFL);
- bool ok = 0 <= fd_flags && 0 <= fcntl (fd, F_SETFL, fd_flags & ~O_NONBLOCK);
+ ok = 0 <= fd_flags && 0 <= fcntl (fd, F_SETFL, fd_flags & ~O_NONBLOCK);
if (ok)
ok = do_wipefd (fd, qname, s, flags);
else
@@ -1178,7 +1209,11 @@ wipefile (char *name, char const *qname,
ok = false;
}
if (ok && flags->remove_file)
- ok = wipename (name, qname, flags);
+ ok = wipename (dirfd, name, qname, flags);
+
+ done:
+ if (dirfd != AT_FDCWD)
+ close (dirfd);
return ok;
}
diff --git a/tests/local.mk b/tests/local.mk
index 77cb83439..840c8118c 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -452,6 +452,7 @@ all_tests = \
tests/shred/shred-passes.sh \
tests/shred/shred-remove.sh \
tests/shred/shred-size.sh \
+ tests/shred/unlink-race.sh \
tests/shuf/shuf.sh \
tests/shuf/shuf-reservoir.sh \
tests/misc/sleep.sh \
diff --git a/tests/shred/unlink-race.sh b/tests/shred/unlink-race.sh
new file mode 100755
index 000000000..8ccb3a7e3
--- /dev/null
+++ b/tests/shred/unlink-race.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+# Test for races when 'shred' removes the file.
+
+# 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_ shred
+
+mkfifo_or_skip_ fifo
+
+# Setup d1/f, which will be removed, and d2/f,
+# which will remain if there are no races.
+mkdir d1 d2 &&
+echo deleted >d1/f &&
+echo saved >d2/f &&
+cp d2/f exp-saved || framework_failure_
+
+# Make sure the 'shred' process gets cleaned up.
+cleanup_() { kill "$pid" 2>/dev/null; wait "$pid" 2>/dev/null; pid=; }
+
+# Use the FIFO as the random source. After opening d1/f1, it
+# will wait for bytes from the FIFO during its random pass.
+shred -v -x -n1 -u --random-source=fifo d1/f 2>err & pid=$!
+exec 3>fifo || framework_failure_
+
+# Once the FIFO is opened by a reader, 'shred' will begin its random pass.
+opened_() { sleep $1; grep 'pass 1/1 (random)\.\.\.$' err >/dev/null; }
+retry_delay_ opened_ .1 6 || framework_failure_
+
+# Move the directory containing the file being overwritten,
+# and create a symbolic link in its place.
+mv d1 d1.orig && ln -s d2 d1 || framework_failure_
+
+# Fill the random source to allow 'shred' to complete.
+timeout 10 cat /dev/zero >&3
+exec 3>&- || framework_failure_
+
+wait "$pid" || fail=1
+pid=
+
+# Verify that the 'shred' removed the file from the original
+# directory and not the one now referenced by the symbolic link.
+compare exp-saved d2/f || fail=1
+test -f d1.orig/f && fail=1
+
+Exit $fail
--
2.56.0
>From 7b48dd18cda41f6aa23732df5efc09815b7e5722 Mon Sep 17 00:00:00 2001
Message-ID: <7b48dd18cda41f6aa23732df5efc09815b7e5722.1793767111.git.collin.fu...@gmail.com>
In-Reply-To: <bfceb456371d219117e63702e149a9bdc992ea55.1793767111.git.collin.fu...@gmail.com>
References: <bfceb456371d219117e63702e149a9bdc992ea55.1793767111.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Tue, 3 Nov 2026 20:19:47 -0800
Subject: [PATCH 2/2] tests: shred: check the behavior when close fails
* tests/shred/failed-close.sh: New file.
* tests/local.mk (all_tests): Add the new test case.
---
tests/local.mk | 1 +
tests/shred/failed-close.sh | 43 +++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)
create mode 100755 tests/shred/failed-close.sh
diff --git a/tests/local.mk b/tests/local.mk
index 840c8118c..55e46e762 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -447,6 +447,7 @@ all_tests = \
tests/cksum/sha384sum.pl \
tests/cksum/sha512sum.pl \
tests/cksum/cksum-sha3.sh \
+ tests/shred/failed-close.sh \
tests/shred/fifo.sh \
tests/shred/shred-exact.sh \
tests/shred/shred-passes.sh \
diff --git a/tests/shred/failed-close.sh b/tests/shred/failed-close.sh
new file mode 100755
index 000000000..f9da43ebe
--- /dev/null
+++ b/tests/shred/failed-close.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+# Test that 'shred' behaves properly when failing to close the
+# file descriptor used to write.
+
+# 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_ shred
+require_strace_ close
+getlimits_
+
+echo a >a || framework_failure_
+
+block_close ()
+{
+ strace --quiet=all -o /dev/null -P a -e inject=close:error=EIO "$@"
+}
+
+# Make sure we could inject a failed close on the selected path.
+block_close true ||
+ skip_ 'strace does not support the required options'
+
+# Inject a failed close on the file we are shredding.
+returns_ 1 block_close shred -n 1 -u a >out 2>err || fail=1
+
+echo "shred: a: failed to close: $EIO" >exp-err || framework_failure_
+compare /dev/null out || fail=1
+compare exp-err err || fail=1
+
+Exit $fail
--
2.56.0
