Re: dd skip bug?

[Jim Meyering]

Pádraig Brady <address@hidden> 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?= <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: 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