bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: dd skip bug?


From: Pádraig Brady
Subject: Re: dd skip bug?
Date: Tue, 27 Jan 2009 11:21:08 +0000
User-agent: Thunderbird 2.0.0.6 (X11/20071008)

Jim Meyering wrote:
> Pádraig Brady <address@hidden> wrote:
> ...
>> I changed message as shown below.
>> But in further testing before commiting I noticed an inconsistency
>> with my fix for this case, specifically when dealing with blocksizes > 1.
>>
>> For seekable files I count a partial block as an unread record,
>> thus printing the diagnostic. This is not the case for pipes though
>> as demonstrated here:
> 
> Good catch.
> 
>> $ truncate -s3 pb.size
>>
>> $ dd iflag=fullblock bs=2 skip=2 if=pb.size
>> ./dd: `pb.size': skip offset was past end of input file: Invalid argument
>>
>> $ cat pb.size | ./dd iflag=fullblock bs=2 skip=2
>>
>> $ cat pb.size | ./dd iflag=fullblock bs=1 skip=4
>> ./dd: `standard input': skip offset was past end of input file: Invalid 
>> argument
>>
>> So should the first dd command not print the message,
>> or should the second dd command print the message?
> 
> Whatever you prefer, as long as it's consistent.

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.

>> I leaning towards changing skip() for seekable files
>> to return the number of _full_ records unskipped
>> to keep it the same as for pipes.
>> Then printing the warning for all cases where
>> the input offset != skip*bs ?

Note you originally added input_offset in 6f31b14a, to deal with
skipping over unreadable parts of _seekable_ files. However I don't
see any problem overloading it to record the current offset of all inputs?

> 
> But remember that the initial offset for a seekable file
> need not be zero.  I.e., this should now evoke a warning:
> 
>   printf 1234 > k && (dd bs=1 skip=2 count=0 && dd bs=1 skip=3) < k

Yep, that was missed in my last patch, but fixed in the patch below
which passes the following:

# Turn dd skip past EOF warnings into errors to simplify testing
DD() {
  ./dd status=noxfer iflag=fullblock $* 2>&1 |
  grep "skip offset was past end of input file" >/dev/null\
  && return 1 || return 0
}
./truncate -s3 dd.three
DD bs=1 skip=3 if=dd.three || fail=1
DD bs=2 skip=2 if=dd.three && fail=1
DD bs=2 skip=3 if=dd.three && fail=1
cat dd.three | DD bs=1 skip=3 || fail=1
cat dd.three | DD bs=2 skip=2 && fail=1
cat dd.three | DD bs=2 skip=3 && fail=1
(DD bs=1 skip=1 count=0 && DD bs=1 skip=2) <dd.three || fail=1
(DD bs=1 skip=1 count=0 && DD bs=1 skip=3) <dd.three && fail=1

cheers,
Pádraig



diff --git a/src/dd.c b/src/dd.c
index d683c5d..4184297 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,28 @@ 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.  */
+      if (us_blocks || (!input_offset_overflow && us_bytes))
+       {
+         if (!us_blocks && !input_seekable && !(iread_fnc == iread_fullblock))
+           {
+             error (0, 0,
+                    _("%s: unable to skip to requested offset"),
+                    quote (input_file));
+           }
+         else
+           {
+             error (0, EINVAL,
+                    _("%s: skip offset was past end of input file"),
+                    quote (input_file));
+           }
+       }
     }

   if (seek_records != 0)
@@ -1778,7 +1845,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)




reply via email to

[Prev in Thread] Current Thread [Next in Thread]