Re: dd skip bug?

[Pádraig Brady]

Jim Meyering wrote:
> Pádraig Brady <address@hidden> wrote:
>> Pádraig Brady wrote:
>>> Well it's more awkward to output the warning for all cases,
>>> as currently we can't distinguish between short reads from
>>> a pipe, and a partial block at the end of the stream.
>>> Thus the error message ".. past end of input file"
>>> is not appropriate for both cases.
>>> Therefore I split out the 2 error cases in the patch below.
>>> It's more consistent but I'm a little worried it's too involved.
>> It is too involved actually. I'll just do a more generic error message:
>>
>> if (us_blocks || (!input_offset_overflow && us_bytes))
>>   {
>>     error (0, 0,
>>            _("%s: unable to skip to requested offset"),
>>            quote (input_file));
>>   }
> 
> Sounds good.  Thanks.
> 

OK going to push the attached soon.

thanks,
Pádraig.

>From 8ae1e0cb42264ce21aec5a59dffa3150b26d769f Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <address@hidden>
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 to specified offset"); 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: Summarize this change in behavior.
* 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                     |   77 +++++++++++++++++++++++++++++++----
 tests/Makefile.am            |    2 +
 tests/dd/skip-seek-past-dev  |   63 +++++++++++++++++++++++++++++
 tests/dd/skip-seek-past-file |   91 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 229 insertions(+), 8 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..afc5148 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -200,8 +200,7 @@ static bool input_seekable;
 static int input_seek_errno;
 
 /* File offset of the input, in bytes, along with a flag recording
-   whether it overflowed.  The offset is valid only if the input is
-   seekable and if the offset has not overflowed.  */
+   whether it overflowed.  */
 static uintmax_t input_offset;
 static bool input_offset_overflow;
 
@@ -1259,12 +1258,62 @@ 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 < (input_offset + offset))
+            {
+              /* When skipping past EOF, return the number of _full_ blocks
+               * that are not skipped, and set offset to EOF, so the caller
+               * can determine the requested skip was not satisfied.  */
+              records = ( offset - st.st_size ) / blocksize;
+              offset = st.st_size - input_offset;
+            }
+          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)
+           {
+             /* The original seek was not attempted as offset > OFF_T_MAX.
+                We should error for write as can't get to the 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 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 quit.  */
+         quit (EXIT_FAILURE);
+       }
+      /* else file_size && offset > OFF_T_MAX or file ! seekable */
 
       do
        {
@@ -1537,10 +1586,22 @@ dd_copy (void)
 
   if (skip_records != 0)
     {
-      skip (STDIN_FILENO, input_file, skip_records, input_blocksize, ibuf);
+      uintmax_t us_bytes = input_offset + (skip_records * input_blocksize);
+      uintmax_t us_blocks = skip (STDIN_FILENO, input_file,
+                                 skip_records, input_blocksize, ibuf);
+      us_bytes -= input_offset;
+
       /* 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.  */
+        asked to skip past EOF, so I assume it's non-fatal.
+        There are 3 reasons why there might be unskipped blocks/bytes:
+            1. file is too small
+            2. pipe has not enough data
+            3. short reads  */
+      if (us_blocks || (!input_offset_overflow && us_bytes))
+       {
+         error (0, 0,
+                _("%s: cannot skip to specified offset"), quote (input_file));
+       }
     }
 
   if (seek_records != 0)
@@ -1778,7 +1839,7 @@ main (int argc, char **argv)
 
   offset = lseek (STDIN_FILENO, 0, SEEK_CUR);
   input_seekable = (0 <= offset);
-  input_offset = offset;
+  input_offset = MAX(0, offset);
   input_seek_errno = errno;
 
   if (output_file == NULL)
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..04101d2
--- /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'
+
+dev_size=$(get_device_size "$device") ||
+  skip_test_ "failed to determine size of $device"
+
+# 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..cfc1439
--- /dev/null
+++ b/tests/dd/skip-seek-past-file
@@ -0,0 +1,91 @@
+#!/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
+
+echo "\
+dd: \`standard input': cannot skip to specified offset
+0+0 records in
+0+0 records out" > skip_err || framework_failure
+
+# skipping beyond number of blocks in file should issue a warning
+dd bs=1 skip=5 count=0 status=noxfer < file 2> err || fail=1
+compare skip_err err || fail=1
+
+# skipping beyond number of bytes in file should issue a warning
+dd bs=3 skip=2 count=0 status=noxfer < file 2> err || fail=1
+compare skip_err err || fail=1
+
+# skipping beyond number of blocks in pipe should issue a warning
+cat file | dd bs=1 skip=5 count=0 status=noxfer 2> err || fail=1
+compare skip_err err || fail=1
+
+# skipping beyond number of bytes in pipe should issue a warning
+cat file | dd bs=3 skip=2 count=0 status=noxfer 2> err || fail=1
+compare skip_err err || fail=1
+
+# Check seeking beyond file already offset into
+# skipping beyond number of blocks in file should issue a warning
+(dd bs=1 skip=1 count=0 2>/dev/null &&
+ dd bs=1 skip=4 status=noxfer 2> err) < file || fail=1
+compare skip_err err || fail=1
+
+# Check seeking beyond file already offset into
+# skipping beyond number of bytes in file should issue a warning
+(dd bs=1 skip=1 count=0 2>/dev/null &&
+ dd bs=2 skip=2 status=noxfer 2> err) < file || fail=1
+compare skip_err 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
-- 
1.5.3.6