Pádraig Brady <[email protected]> wrote: > Following is the latest version. I should mention that > I think the only possibly contentious part of this is > the FIXME comment I addressed where a warning is now printed > if you skip past EOF, as demonstrated by this command: > > $ printf %4s | dd bs=1 skip=5 && \ > echo only warning, but mixed with status > > ./dd: `standard input': cannot skip: Invalid argument > 0+0 records in > 0+0 records out > 0 bytes (0 B) copied, 0.00143536 s, 0.0 kB/s > only warning, but mixed with status
Thanks for being patient ;-) This patch looks fine, modulo a few nits and suggestions. I presume you've tested it on a 32-bit system (I haven't). I like the use of timeout to enforce the "quickly" test requirement. >>From 3f1d6e479095a0d5f8c52f82af020181a9380305 Mon Sep 17 00:00:00 2001 > From: =?utf-8?q?P=C3=A1draig=20Brady?= <[email protected]> > Date: Thu, 20 Nov 2008 10:28:31 +0000 > Subject: [PATCH] dd: Better handle user specified offsets that are too big > > Following are the before and after operations for seekable files, > for the various erroneous offsets handled by this patch: > > skip beyond end of file > before: immediately exit(0); > after : immediately printf("cannot skip: Invalid argument); exit(0); > > skip > max file size > before: read whole file and exit(0); > after : immediately printf("cannot skip: Invalid argument); exit(1); > seek > max file size > before: immediately printf("truncate error: EFBIG"); exit(1); > after : immediately printf("truncate error: EFBIG"); exit(1); > > skip > OFF_T_MAX > before: read whole device/file and exit(0); > after : immediately printf("cannot skip:"); exit(1); > seek > OFF_T_MAX > before: immediately printf("truncate error: offset too large"); exit(1); > after : immediately printf("truncate error: offset too large"); exit(1); > > skip > device size > before: read whole device and exit(0); > after : immediately printf("cannot skip: Invalid argument); exit(1); > seek > device size > before: read whole device and printf("write error: ENOSPC"); exit(1); > after : immediately printf("cannot seek: Invalid argument); exit(1); > > * NEWS: Summarise this change in behaviour. Please use american-english spelling: s/ise/ize/ s/iour/ior/ > * src/dd.c (skip): Add error checking for large seek/skip offsets on > seekable files, rather than deferring to using read() to advance offset. > (dd_copy): Print a warning if skip past EOF, as per FIXME comment. > * test/Makefile.am: Add 2 new tests. > * tests/dd/seek-skip-past-file: Add tests for first 3 cases above. > * tests/dd/seek-skip-past-dev: Add root only test for last case above. > --- > NEWS | 4 ++ > src/dd.c | 55 +++++++++++++++++++++++++++++++++-- > tests/Makefile.am | 2 + > tests/dd/skip-seek-past-dev | 63 ++++++++++++++++++++++++++++++++++++++++ > tests/dd/skip-seek-past-file | 65 > ++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 185 insertions(+), 4 deletions(-) > create mode 100755 tests/dd/skip-seek-past-dev > create mode 100755 tests/dd/skip-seek-past-file > > diff --git a/NEWS b/NEWS > index 83b8ed9..99fc182 100644 > --- a/NEWS > +++ b/NEWS > @@ -38,6 +38,10 @@ GNU coreutils NEWS -*- > outline -*- > cp and mv: the --reply={yes,no,query} option has been removed. > Using it has elicited a warning for the last three years. > > + dd: user specified offsets that are too big are handled better. > + Previously, erroneous parameters to skip and seek could result > + in redundant reading of the file with no warnings or errors. > + > du: -H (initially equivalent to --si) is now equivalent to > --dereference-args, and thus works as POSIX requires > > diff --git a/src/dd.c b/src/dd.c > index d683c5d..30d06df 100644 > --- a/src/dd.c > +++ b/src/dd.c > @@ -1259,12 +1259,56 @@ skip (int fdesc, char const *file, uintmax_t records, > size_t blocksize, > && 0 <= skip_via_lseek (file, fdesc, offset, SEEK_CUR)) > { > if (fdesc == STDIN_FILENO) > - advance_input_offset (offset); > - return 0; > + { > + struct stat st; > + if (fstat (STDIN_FILENO, &st) != 0) > + error (EXIT_FAILURE, errno, _("cannot fstat %s"), quote (file)); > + if (S_ISREG (st.st_mode) && st.st_size < offset) > + records = ( offset - st.st_size + blocksize - 1 ) / blocksize; > + else > + records = 0; > + advance_input_offset (offset); > + } > + else > + records = 0; > + return records; > } > else > { > int lseek_errno = errno; > + off_t soffset; > + > + /* The seek request may have failed above if it was too big > + (> device size, > max file size, etc.) > + Or it may not have been done at all (> OFF_T_MAX). > + Therefore try to seek to the end of the file, > + to avoid redundant reading. */ > + if ((soffset = skip_via_lseek (file, fdesc, 0, SEEK_END)) >= 0) > + { > + /* File is seekable, and we're at the end of it, and > + size <= OFF_T_MAX. So there's no point using read to advance. */ > + > + if (!lseek_errno) > + { > + /* Orig seek not attempted as offset > OFF_T_MAX Please spell out "original", and use good grammar in comments, e.g., The original seek was ... > + We should error for write as can't get to desired location, > + even if OFF_T_MAX < max file size. > + For read we're not going to read any data anyway, > + so we should error for consistency. > + It would be nice to not error for /dev/{zero,null} > + for any offset, but that's not significant issue I think. */ i.e., add "a" here, and you can drop the "I think" for any offset, but that's not a significant issue. */ > + lseek_errno = EOVERFLOW; > + } > + > + if (fdesc == STDIN_FILENO) > + error (0, lseek_errno, _("%s: cannot skip"), quote (file)); > + else > + error (0, lseek_errno, _("%s: cannot seek"), quote (file)); > + /* If the file has a specific size and we've asked > + to skip/seek beyond the max allowable, then should quit. */ s/should // > + quit (EXIT_FAILURE); > + } > + /* else file_size && offset > OFF_T_MAX or file ! seekable */ > > do > { > @@ -1537,10 +1581,13 @@ dd_copy (void) > > if (skip_records != 0) > { > - skip (STDIN_FILENO, input_file, skip_records, input_blocksize, ibuf); > + uintmax_t unskipped = skip (STDIN_FILENO, input_file, > + skip_records, input_blocksize, ibuf); > /* POSIX doesn't say what to do when dd detects it has been > asked to skip past EOF, so I assume it's non-fatal if the > - call to 'skip' returns nonzero. FIXME: maybe give a warning. */ > + call to 'skip' returns nonzero. */ > + if (unskipped) > + error (0, EINVAL, _("%s: cannot skip"), quote (input_file)); How about a diagnostic giving more detail? I.e., saying that the skip offset was larger than the length of the input file. And maybe a different (non-negative) variable name? Also, there's no need to use uintmax_t. E.g., bool skip_ok = (skip (...) == 0); then test if (!skip_ok) > } > > if (seek_records != 0) > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 6dce9cd..69475ad 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -24,6 +24,7 @@ root_tests = \ > cp/cp-a-selinux \ > cp/preserve-gid \ > cp/special-bits \ > + dd/skip-seek-past-dev \ > ls/capability \ > ls/nameless-uid \ > misc/chcon \ > @@ -287,6 +288,7 @@ TESTS = \ > dd/reblock \ > dd/skip-seek \ > dd/skip-seek2 \ > + dd/skip-seek-past-file \ > dd/unblock-sync \ > df/total-verify \ > du/2g \ > diff --git a/tests/dd/skip-seek-past-dev b/tests/dd/skip-seek-past-dev > new file mode 100755 > index 0000000..906344c > --- /dev/null > +++ b/tests/dd/skip-seek-past-dev > @@ -0,0 +1,63 @@ > +#!/bin/sh > +# test diagnostics are printed immediately when seeking beyond device. > + > +# Copyright (C) 2008 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 <http://www.gnu.org/licenses/>. > + > +if test "$VERBOSE" = yes; then > + set -x > + dd --version > +fi > + > +. $srcdir/test-lib.sh > + > +# need write access to device > +# (even though we don't actually write anything) > +require_root_ > + > +get_device_size() { > + BLOCKDEV=blockdev > + $BLOCKDEV -V >/dev/null 2>&1 || BLOCKDEV=/sbin/blockdev > + $BLOCKDEV --getsize64 "$1" > +} > + > +fail=0 > + > +# Get path to device the current dir is on. > +# Note df can only get fs size, not device size. > +device=$(df -P --local . | tail -n1 | cut -d' ' -f1) || > +skip_test_ 'this test runs only on local file systems' Please indent the conditional part: device=$(df -P --local . | tail -n1 | cut -d' ' -f1) || skip_test_ 'this test runs only on local file systems' > + > +dev_size=$(get_device_size "$device") || > +skip_test_ "Could not determine size of $device" here too. s/Could not/failed to/ > +# Don't use shell arithimetic as older version of dash use longs > +DEV_OFLOW=$(expr $dev_size + 1) > + > +timeout 1 dd bs=1 skip=$DEV_OFLOW count=0 status=noxfer < "$device" 2> err > +test "$?" = "1" || fail=1 > +echo "dd: \`standard input': cannot skip: Invalid argument > +0+0 records in > +0+0 records out" > err_ok || framework_failure > +compare err_ok err || fail=1 > + > +timeout 1 dd bs=1 seek=$DEV_OFLOW count=0 status=noxfer > "$device" 2> err > +test "$?" = "1" || fail=1 > +echo "dd: \`standard output': cannot seek: Invalid argument > +0+0 records in > +0+0 records out" > err_ok || framework_failure > +compare err_ok err || fail=1 > + > +Exit $fail > diff --git a/tests/dd/skip-seek-past-file b/tests/dd/skip-seek-past-file > new file mode 100755 > index 0000000..dc02642 > --- /dev/null > +++ b/tests/dd/skip-seek-past-file > @@ -0,0 +1,65 @@ > +#!/bin/sh > +# test diagnostics are printed when seeking too far in seekable files. > + > +# Copyright (C) 2008 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 <http://www.gnu.org/licenses/>. > + > +if test "$VERBOSE" = yes; then > + set -x > + dd --version > +fi > + > +. $srcdir/test-lib.sh > +eval $(getlimits) #for OFF_T limits > + > +fail=0 > + > +printf "1234" > file || framework_failure > + > +# skipping beyond end of file should issue a warning > +dd bs=1 skip=5 count=0 status=noxfer < file 2> err || fail=1 > +echo "dd: \`standard input': cannot skip: Invalid argument > +0+0 records in > +0+0 records out" > err_ok || framework_failure > +compare err_ok err || fail=1 > + > +# seeking beyond end of file is OK > +dd bs=1 seek=5 count=0 status=noxfer > file 2> err || fail=1 > +echo "0+0 records in > +0+0 records out" > err_ok || framework_failure > +compare err_ok err || fail=1 > + > +# skipping > OFF_T_MAX should fail immediately > +dd bs=1 skip=$OFF_T_OFLOW count=0 status=noxfer < file 2> err && fail=1 > +echo "dd: \`standard input': cannot skip: Value too large for defined data > type > +0+0 records in > +0+0 records out" > err_ok || framework_failure > +compare err_ok err || fail=1 > + > +# skipping > max file size should fail immediately > +# Note I'm guessing there is a small chance that an lseek() could actually > work > +# and only a write() would fail (with EFBIG) when offset > max file size. > +# So this test will both test for that, and ensure that dd > +# exits immediately with an appropriate error when lseek() does error. > +if ! truncate --size=$OFF_T_MAX in 2>/dev/null; then > + # truncate is to ensure file system doesn't actually support OFF_T_MAX > files > + dd bs=1 skip=$OFF_T_MAX count=0 status=noxfer < file 2> err && fail=1 > + echo "dd: \`standard input': cannot skip: Invalid argument > +0+0 records in > +0+0 records out" > err_ok || framework_failure > + compare err_ok err || fail=1 > +fi > + > +Exit $fail _______________________________________________ Bug-coreutils mailing list [email protected] http://lists.gnu.org/mailman/listinfo/bug-coreutils
