Re: dd skip bug?

[Pádraig Brady]

I spent a little more time on this issue.
The attached patch makes the following changes for seekable inputs and outputs.
Note skip is for input, and seek is for output in the following:

skip beyond end of file
  before: immediately exit(0);
  after : immediately printf("cannot skip: Invalid argument); exit(0);

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);

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);


If the above is desired then I'll add tests before checking in.

cheers,
Pádraig.

>From 3eaba02cb73bfc45b00d7e55cb6daa1f1b2e4ba2 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] Add seek/skip error checking to dd

*src/dd.c: Add seek error checking to dd.
Previously it would have redundantly tried
to read to the invalid offset and then output
neither data or an error message.
---
 src/dd.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/src/dd.c b/src/dd.c
index f598e44..c4b114c 100644
--- a/src/dd.c
+++ b/src/dd.c
@@ -1249,12 +1249,52 @@ 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 is no point using read to advance.
+
+            if (!lseek_errno) then orig seek not attempted as offset > 
OFF_T_MAX
+            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.  */
+
+         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.  */
+         quit (EXIT_FAILURE);
+       }
+      /* else file_size && offset > OFF_T_MAX or file ! seekable */
 
       do
        {
@@ -1527,10 +1567,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));
     }
 
   if (seek_records != 0)
-- 
1.5.3.6