Paul Eggert <[email protected]> writes:
> On 2025-12-24 08:08, Collin Funk wrote:
>> Pádraig Brady<[email protected]> writes:
>>> I wonder do we want to do the dd_copy() though if the ftruncate failed,
>>> as we'd be copying data at the wrong offset then?
>>>
>>> Perhaps instead we might want:
>>>
>>> if (! exit_status)
>>> exit_status = dd_copy ();
>> Paul, what do you think?
>> Based on your commit message it seems like you wanted to do the copy
>> as
>> long as we have the file open [1]?
>> Collin
>> [1]https://github.com/coreutils/coreutils/
>> commit/50e438972b8f6e4c7486c38beb542539de0298c7
>
> As I recall, [1] merely is about doing a fsync of whatever we've
> already written, even if there was an error after we wrote it. The
> idea is that the fsync is not *further* copying; it's merely ensuring
> that the *previous* copying is fsynced.
Makes sense.
> ftruncate, on the other hand, is *further* copying, so if it fails
> then (as POSIX requires[1]) there should be no further attempt at
> copying, and a diagnostic should be written and the exit status should
> be nonzero.
I see. Also in the description of "of=file" it says:
If seek=expr is specified, but conv=notrunc is not, the effect of
the copy shall be to preserve the blocks in the output file over
which dd seeks, but no other portion of the output file shall be
preserved.
So, based on my interpretation, when this ftruncate fails the output
file, if it exists, should not be modified, since it occurs before the
copy.
This v2 patch also checks for that behavior. I will leave it a bit for
review.
Collin
>From 52a4222df87ff94951e894f1cc3282bafbd8238c Mon Sep 17 00:00:00 2001
Message-ID: <52a4222df87ff94951e894f1cc3282bafbd8238c.1766644867.git.collin.fu...@gmail.com>
From: Collin Funk <[email protected]>
Date: Tue, 23 Dec 2025 21:45:37 -0800
Subject: [PATCH v2] dd: don't continue copying when ftruncate fails using
seek= and of=
* src/dd.c (main): Reduce the scope of exit_status. Exit immediately if
ftruncate fails.
* tests/dd/fail-ftruncate-fstat.sh: New test.
* tests/local.mk (all_tests): Add the new test.
* NEWS: Mention the bug fix.
---
NEWS | 4 ++
src/dd.c | 22 +++------
tests/dd/fail-ftruncate-fstat.sh | 76 ++++++++++++++++++++++++++++++++
tests/local.mk | 1 +
4 files changed, 88 insertions(+), 15 deletions(-)
create mode 100755 tests/dd/fail-ftruncate-fstat.sh
diff --git a/NEWS b/NEWS
index 58cf776a9..206019785 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS -*- outline -*-
** Bug fixes
+ 'dd seek=N of=FILE' no longer continues copying, overwriting FILE if it
+ exists, if ftruncate fails.
+ [bug introduced in coreutils-9.1]
+
du and ls no longer modify strings returned by getenv.
POSIX says this is not portable.
[bug introduced in fileutils-4.1.6]
diff --git a/src/dd.c b/src/dd.c
index 117d61908..3c79ad81c 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -2379,7 +2379,6 @@ synchronize_output (void)
int
main (int argc, char **argv)
{
- int exit_status;
off_t offset;
install_signal_handlers ();
@@ -2472,20 +2471,17 @@ main (int argc, char **argv)
int ftruncate_errno = errno;
struct stat stdout_stat;
if (ifstat (STDOUT_FILENO, &stdout_stat) != 0)
- {
- diagnose (errno, _("cannot fstat %s"), quoteaf (output_file));
- exit_status = EXIT_FAILURE;
- }
+ error (EXIT_FAILURE, errno, _("cannot fstat %s"),
+ quoteaf (output_file));
else if (S_ISREG (stdout_stat.st_mode)
|| S_ISDIR (stdout_stat.st_mode)
|| S_TYPEISSHM (&stdout_stat))
{
intmax_t isize = size;
- diagnose (ftruncate_errno,
- _("failed to truncate to %jd bytes"
- " in output file %s"),
- isize, quoteaf (output_file));
- exit_status = EXIT_FAILURE;
+ error (EXIT_FAILURE, ftruncate_errno,
+ _("failed to truncate to %jd bytes"
+ " in output file %s"),
+ isize, quoteaf (output_file));
}
}
}
@@ -2494,11 +2490,7 @@ main (int argc, char **argv)
start_time = gethrxtime ();
next_time = start_time + XTIME_PRECISION;
- exit_status = dd_copy ();
-
- int sync_status = synchronize_output ();
- if (sync_status)
- exit_status = sync_status;
+ int exit_status = dd_copy () | synchronize_output ();
if (max_records == 0 && max_bytes == 0)
{
diff --git a/tests/dd/fail-ftruncate-fstat.sh b/tests/dd/fail-ftruncate-fstat.sh
new file mode 100755
index 000000000..994051f17
--- /dev/null
+++ b/tests/dd/fail-ftruncate-fstat.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+# Check that 'dd' does not continue copying if ftruncate and fstat fail.
+
+# Copyright (C) 2025 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_ dd
+require_gcc_shared_
+
+cat > k.c <<'EOF' || framework_failure_
+#include <sys/stat.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+
+int
+ftruncate (int fd, off_t length)
+{
+ /* Prove that LD_PRELOAD works: create the evidence file "x". */
+ fclose (fopen ("x", "w"));
+
+ /* Pretend failure. */
+ errno = EPERM;
+ return -1;
+}
+
+int
+fstat (int fd, struct stat *statbuf)
+{
+ /* Prove that LD_PRELOAD works: create the evidence file "y". */
+ fclose (fopen ("y", "w"));
+
+ /* Pretend failure. */
+ errno = EPERM;
+ return -1;
+}
+EOF
+
+# Then compile/link it:
+gcc_shared_ k.c k.so \
+ || framework_failure_ 'failed to build shared library'
+
+# Setup the file "out" and preserve it's original contents in "exp-out".
+yes | head -n 2048 | tr -d '\n' > out || framework_failure_
+cp out exp-out || framework_failure_
+
+LD_PRELOAD=$LD_PRELOAD:./k.so dd if=/dev/zero of=out count=1 \
+ seek=1 status=none 2>err
+ret=$?
+
+test -f x && test -f y \
+ || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
+
+# After ftruncate fails, we use fstat to get the file type.
+echo "dd: cannot fstat 'out': Operation not permitted" > exp
+compare exp err || fail=1
+
+# coreutils 9.1 to 9.9 would mistakenly continue copying after ftruncate
+# failed and exit successfully.
+test "$ret" = 1 || fail=1
+compare exp-out out || fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 59aa18adf..56a37c524 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -583,6 +583,7 @@ all_tests = \
tests/dd/ascii.sh \
tests/dd/conv-case.sh \
tests/dd/direct.sh \
+ tests/dd/fail-ftruncate-fstat.sh \
tests/dd/misc.sh \
tests/dd/no-allocate.sh \
tests/dd/nocache.sh \
--
2.52.0
