[PATCH] dd: fix corrupted/incomplete reads/skips.

[Ralf Friedl]

Nicholas Clark wrote:
> Busybox dd expects that read() calls (as provided by safe_read) will
> always return the requested amount of data unless at EOF. This isn't
> true for safe_read(), but it is true for full_read() (which loops
> over safe_read until enough data has been retrieved).
>
> This patch replaces dd's safe_read() calls with full_read() to make
> sure that the underlying assumptions about read-length always hold
> true.
>
> Without this patch, users on some platforms can get corrupted reads
> or incomplete skips.
>
> Signed-off-by: Nicholas Clark <nicholas.clark at gmail.com 
> <http://gmail.com>>
> ---
>  coreutils/dd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/coreutils/dd.c b/coreutils/dd.c
> index d302f35d3..6fc062d8d 100644
> --- a/coreutils/dd.c
> +++ b/coreutils/dd.c
> @@ -450,7 +450,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
> size_t blocksz = (G.flags & FLAG_SKIP_BYTES) ? 1 : ibs;
> if (lseek(ifd, skip * blocksz, SEEK_CUR) < 0) {
> do {
> -ssize_t n = safe_read(ifd, ibuf, blocksz);
> +ssize_t n = full_read(ifd, ibuf, blocksz);
> if (n < 0)
> goto die_infile;
> if (n == 0)
> @@ -466,7 +466,7 @@ int dd_main(int argc UNUSED_PARAM, char **argv)
> while (!(G.flags & FLAG_COUNT) || (G.in_full + G.in_part != count)) {
> ssize_t n;
> -n = safe_read(ifd, ibuf, ibs);
> +n = full_read(ifd, ibuf, ibs);
> if (n == 0)
> break;
> if (n < 0) {
>
If we take GNU dd as a reference, then dd doesn't expect read() calls to 
return the requested amount of data. The very fact that the message at 
the end says "(full)+(partial) records in/out" shows that partial reads 
are expected to happen. With yout patch, there can never be a partial 
read, except maybe on the last record.

A look at 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/dd.html shows 
that even safe_read is wrong: "For SIGINT, the /dd/ utility shall 
interrupt its current processing, write status information to standard 
error, and exit as though terminated by SIGINT. It shall take the 
standard action for all other signals"
The standard also states: "1. An input block is read. 2. If the input 
block is shorter ...", nothing about forcing the block to the full size.

Your patch changes both the handling of the skip option and the regular 
read. GNU dd uses simple read in both cases, unless the nonstandard 
iflag=fullblock is used. So the proper fix would be to use read as 
default and full_read if iflag=fullblock is specified. Busybox dd 
already has conditional support for iflag, but only for skip_bytes, not 
for fullblock. It should take just a few bytes to add fullblock.

A variation of the test by Denys shows how GNU dd behaves with short 
reads in the skip and data phase:

Without fullblock, output is "34":
$ for i in $(seq 9); do sleep 1; echo -n $i; done | strace -s99 dd bs=2 
skip=2 count=2
read(0, "1", 2)                         = 1
read(0, "2", 2)                         = 1
write(2, "dd: ", 4)                     = 4
write(2, "warning: partial read (1 byte); suggest iflag=fullblock", 55) = 55
write(2, "\n", 1)                       = 1
write(2, "dd: ", 4)                     = 4
write(2, "'standard input': cannot skip to specified offset", 49) = 49
write(2, "\n", 1)                       = 1
read(0, "3", 2)                         = 1
write(1, "3", 13)                        = 1
read(0, "4", 2)                         = 1
write(1, "4", 14)                        = 1
close(0)                                = 0
close(1)                                = 0
write(2, "0+2 records in\n0+2 records out\n", 31) = 31

With fullblock, output is "5678":
read(0, "1", 2)                         = 1
read(0, "2", 1)                         = 1
read(0, "3", 2)                         = 1
read(0, "4", 1)                         = 1
read(0, "5", 2)                         = 1
read(0, "6", 1)                         = 1
write(1, "56", 256)                       = 2
read(0, "7", 2)                         = 1
read(0, "8", 1)                         = 1
write(1, "78", 278)                       = 2
close(0)                                = 0
close(1)                                = 0
write(2, "2+0 records in\n2+0 records out\n", 31) = 31