Bug 189174 - [patch] dd(1): dd conv=sparse bs=64k EINVAL on sparse last block
Summary: [patch] dd(1): dd conv=sparse bs=64k EINVAL on sparse last block
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 9.2-STABLE
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-01 10:20 UTC by Thomas Quinot
Modified: 2016-08-30 12:31 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Quinot 2014-05-01 10:20:00 UTC
	When copying to an md device (or a slice thereof) with conv=sparse
	and an explicit block size, dd will produce an error if the last
	block is a sparse and partial one.

	(This breaks NanoBSD builds in the case where two system images
	are created, and the slice size computed from the media size
	is not an intregral multiple of 64k).

	This is with dd(1) from stable/9 as of rev 265172.

How-To-Repeat: # dd if=/dev/zero of=f1 count=129
129+0 records in
129+0 records out
66048 bytes transferred in 0.003451 secs (19139518 bytes/sec)
# dd if=/dev/zero of=f2 count=129
129+0 records in
129+0 records out
66048 bytes transferred in 0.003485 secs (18952274 bytes/sec)
# mdconfig -f f1
md0
# mdconfig -f f2
md1
# dd if=/dev/md0 of=/dev/md1 conv=sparse bs=64k
dd: /dev/md1: Invalid argument
1+1 records in
1+0 records out
65536 bytes transferred in 0.000338 secs (193849018 bytes/sec)
Comment 1 thomas 2014-05-01 10:28:25 UTC
* FreeBSD-gnats-submit@FreeBSD.org, 2014-05-01 :

> >Category:       bin
> >Responsible:    freebsd-bugs
> >Synopsis:       dd conv=sparse bs=64k EINVAL on sparse last block
> >Arrival-Date:   Thu May 01 09:20:00 UTC 2014

The following fix appears to fix the problem here:

Index: dd.c
===================================================================
--- dd.c	(révision 265172)
+++ dd.c	(copie de travail)
@@ -450,13 +450,16 @@
 				nw = cnt;
 			} else {
 				if (pending != 0) {
-					if (force)
+					/* If forced to write, and we have no data left, we need
+                                         * to write an explicit 0.
+                                         */
+					if (force && cnt == 0)
 						pending--;
 					if (lseek(out.fd, pending, SEEK_CUR) ==
 					    -1)
 						err(2, "%s: seek error creating sparse file",
 						    out.name);
-					if (force)
+					if (force && cnt == 0)
 						write(out.fd, outp, 1);
 					pending = 0;
 				}
Comment 2 dfilter service freebsd_committer freebsd_triage 2014-05-07 20:33:34 UTC
Author: thomas
Date: Wed May  7 19:33:29 2014
New Revision: 265593
URL: http://svnweb.freebsd.org/changeset/base/265593

Log:
  (dd_out): Fix handling of all-zeroes block at end of input with
  conv=sparse.
  
  This change fixes two separate issues observed when the last output
  block is all zeroes, and conv=sparse is in use. In this case, care
  must be taken to roll back the last seek and write the entire last zero
  block at the original offset where it should have occurred: when the
  destination file is a block device, it is not possible to roll back
  by just one character as the write would then not be properly aligned.
  
  Furthermore, the buffer used to write this last all-zeroes block
  needs to be properly zeroed-out. This was not the case previously,
  resulting in a junk data byte appearing instead of a zero in the
  output stream.
  
  PR:		bin/189174
  PR:		bin/189284
  Reviewed by:	kib
  MFC after:	2 weeks

Modified:
  head/bin/dd/dd.c

Modified: head/bin/dd/dd.c
==============================================================================
--- head/bin/dd/dd.c	Wed May  7 19:30:28 2014	(r265592)
+++ head/bin/dd/dd.c	Wed May  7 19:33:29 2014	(r265593)
@@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/disklabel.h>
 #include <sys/filio.h>
 
+#include <assert.h>
 #include <ctype.h>
 #include <err.h>
 #include <errno.h>
@@ -77,6 +78,7 @@ STAT	st;			/* statistics */
 void	(*cfunc)(void);		/* conversion function */
 uintmax_t cpy_cnt;		/* # of blocks to copy */
 static off_t	pending = 0;	/* pending seek if sparse */
+static off_t	last_sp = 0;	/* size of last added sparse block */
 u_int	ddflags = 0;		/* conversion options */
 size_t	cbsz;			/* conversion block size */
 uintmax_t files_cnt = 1;	/* # of files to copy */
@@ -174,6 +176,8 @@ setup(void)
 	} else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL ||
 	    (out.db = malloc(out.dbsz + cbsz)) == NULL)
 		err(1, "output buffer");
+
+	/* dbp is the first free position in each buffer. */
 	in.dbp = in.db;
 	out.dbp = out.db;
 
@@ -436,8 +440,15 @@ dd_out(int force)
 	 * we play games with the buffer size, and it's usually a partial write.
 	 */
 	outp = out.db;
+
+	/*
+	 * If force, first try to write all pending data, else try to write
+	 * just one block. Subsequently always write data one full block at
+	 * a time at most.
+	 */
 	for (n = force ? out.dbcnt : out.dbsz;; n = out.dbsz) {
-		for (cnt = n;; cnt -= nw) {
+		cnt = n;
+		do {
 			sparse = 0;
 			if (ddflags & C_SPARSE) {
 				sparse = 1;	/* Is buffer sparse? */
@@ -449,18 +460,24 @@ dd_out(int force)
 			}
 			if (sparse && !force) {
 				pending += cnt;
+				last_sp = cnt;
 				nw = cnt;
 			} else {
 				if (pending != 0) {
-					if (force)
-						pending--;
+					/* If forced to write, and we have no
+					 * data left, we need to write the last
+					 * sparse block explicitly.
+					 */
+					if (force && cnt == 0) {
+						pending -= last_sp;
+						assert(outp == out.db);
+						memset(outp, 0, cnt);
+					}
 					if (lseek(out.fd, pending, SEEK_CUR) ==
 					    -1)
 						err(2, "%s: seek error creating sparse file",
 						    out.name);
-					if (force)
-						write(out.fd, outp, 1);
-					pending = 0;
+					pending = last_sp = 0;
 				}
 				if (cnt)
 					nw = write(out.fd, outp, cnt);
@@ -475,27 +492,29 @@ dd_out(int force)
 					err(1, "%s", out.name);
 				nw = 0;
 			}
+
 			outp += nw;
 			st.bytes += nw;
-			if ((size_t)nw == n) {
-				if (n != out.dbsz)
-					++st.out_part;
-				else
-					++st.out_full;
-				break;
-			}
-			++st.out_part;
-			if ((size_t)nw == cnt)
-				break;
-			if (out.flags & ISTAPE)
-				errx(1, "%s: short write on tape device",
-				    out.name);
-			if (out.flags & ISCHR && !warned) {
-				warned = 1;
-				warnx("%s: short write on character device",
-				    out.name);
+
+			if ((size_t)nw == n && n == out.dbsz)
+				++st.out_full;
+			else
+				++st.out_part;
+
+			if ((size_t) nw != cnt) {
+				if (out.flags & ISTAPE)
+					errx(1, "%s: short write on tape device",
+				    	out.name);
+				if (out.flags & ISCHR && !warned) {
+					warned = 1;
+					warnx("%s: short write on character device",
+				    	out.name);
+				}
 			}
-		}
+
+			cnt -= nw;
+		} while (cnt != 0);
+
 		if ((out.dbcnt -= n) < out.dbsz)
 			break;
 	}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 3 Thomas Quinot freebsd_committer freebsd_triage 2014-05-07 20:54:36 UTC
State Changed
From-To: open->closed

Fixed in head.
Comment 4 dfilter service freebsd_committer freebsd_triage 2014-05-21 08:21:40 UTC
Author: thomas
Date: Wed May 21 07:21:36 2014
New Revision: 266488
URL: http://svnweb.freebsd.org/changeset/base/266488

Log:
  MFC rev. 265593:
  (dd_out): Fix handling of all-zeroes block at end of input with
  conv=sparse.
  
  PR:		bin/189174
  PR:		bin/189284
  Reviewed by:	kib

Modified:
  stable/10/bin/dd/dd.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/bin/dd/dd.c
==============================================================================
--- stable/10/bin/dd/dd.c	Wed May 21 06:33:21 2014	(r266487)
+++ stable/10/bin/dd/dd.c	Wed May 21 07:21:36 2014	(r266488)
@@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/filio.h>
 #include <sys/time.h>
 
+#include <assert.h>
 #include <ctype.h>
 #include <err.h>
 #include <errno.h>
@@ -76,6 +77,7 @@ STAT	st;			/* statistics */
 void	(*cfunc)(void);		/* conversion function */
 uintmax_t cpy_cnt;		/* # of blocks to copy */
 static off_t	pending = 0;	/* pending seek if sparse */
+static off_t	last_sp = 0;	/* size of last added sparse block */
 u_int	ddflags = 0;		/* conversion options */
 size_t	cbsz;			/* conversion block size */
 uintmax_t files_cnt = 1;	/* # of files to copy */
@@ -173,6 +175,8 @@ setup(void)
 	} else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL ||
 	    (out.db = malloc(out.dbsz + cbsz)) == NULL)
 		err(1, "output buffer");
+
+	/* dbp is the first free position in each buffer. */
 	in.dbp = in.db;
 	out.dbp = out.db;
 
@@ -434,8 +438,15 @@ dd_out(int force)
 	 * we play games with the buffer size, and it's usually a partial write.
 	 */
 	outp = out.db;
+
+	/*
+	 * If force, first try to write all pending data, else try to write
+	 * just one block. Subsequently always write data one full block at
+	 * a time at most.
+	 */
 	for (n = force ? out.dbcnt : out.dbsz;; n = out.dbsz) {
-		for (cnt = n;; cnt -= nw) {
+		cnt = n;
+		do {
 			sparse = 0;
 			if (ddflags & C_SPARSE) {
 				sparse = 1;	/* Is buffer sparse? */
@@ -447,18 +458,24 @@ dd_out(int force)
 			}
 			if (sparse && !force) {
 				pending += cnt;
+				last_sp = cnt;
 				nw = cnt;
 			} else {
 				if (pending != 0) {
-					if (force)
-						pending--;
+					/* If forced to write, and we have no
+					 * data left, we need to write the last
+					 * sparse block explicitly.
+					 */
+					if (force && cnt == 0) {
+						pending -= last_sp;
+						assert(outp == out.db);
+						memset(outp, 0, cnt);
+					}
 					if (lseek(out.fd, pending, SEEK_CUR) ==
 					    -1)
 						err(2, "%s: seek error creating sparse file",
 						    out.name);
-					if (force)
-						write(out.fd, outp, 1);
-					pending = 0;
+					pending = last_sp = 0;
 				}
 				if (cnt)
 					nw = write(out.fd, outp, cnt);
@@ -473,27 +490,29 @@ dd_out(int force)
 					err(1, "%s", out.name);
 				nw = 0;
 			}
+
 			outp += nw;
 			st.bytes += nw;
-			if ((size_t)nw == n) {
-				if (n != out.dbsz)
-					++st.out_part;
-				else
-					++st.out_full;
-				break;
-			}
-			++st.out_part;
-			if ((size_t)nw == cnt)
-				break;
-			if (out.flags & ISTAPE)
-				errx(1, "%s: short write on tape device",
-				    out.name);
-			if (out.flags & ISCHR && !warned) {
-				warned = 1;
-				warnx("%s: short write on character device",
-				    out.name);
+
+			if ((size_t)nw == n && n == out.dbsz)
+				++st.out_full;
+			else
+				++st.out_part;
+
+			if ((size_t) nw != cnt) {
+				if (out.flags & ISTAPE)
+					errx(1, "%s: short write on tape device",
+				    	out.name);
+				if (out.flags & ISCHR && !warned) {
+					warned = 1;
+					warnx("%s: short write on character device",
+				    	out.name);
+				}
 			}
-		}
+
+			cnt -= nw;
+		} while (cnt != 0);
+
 		if ((out.dbcnt -= n) < out.dbsz)
 			break;
 	}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 dfilter service freebsd_committer freebsd_triage 2014-05-21 08:42:48 UTC
Author: thomas
Date: Wed May 21 07:42:42 2014
New Revision: 266489
URL: http://svnweb.freebsd.org/changeset/base/266489

Log:
  MFC rev. 265593:
  (dd_out): Fix handling of all-zeroes block at end of input with
  conv=sparse.
  
  PR:		bin/189174
  PR:		bin/189284
  Reviewed by:	kib

Modified:
  stable/9/bin/dd/dd.c
Directory Properties:
  stable/9/   (props changed)
  stable/9/bin/   (props changed)
  stable/9/bin/dd/   (props changed)

Modified: stable/9/bin/dd/dd.c
==============================================================================
--- stable/9/bin/dd/dd.c	Wed May 21 07:21:36 2014	(r266488)
+++ stable/9/bin/dd/dd.c	Wed May 21 07:42:42 2014	(r266489)
@@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/filio.h>
 #include <sys/time.h>
 
+#include <assert.h>
 #include <ctype.h>
 #include <err.h>
 #include <errno.h>
@@ -76,6 +77,7 @@ STAT	st;			/* statistics */
 void	(*cfunc)(void);		/* conversion function */
 uintmax_t cpy_cnt;		/* # of blocks to copy */
 static off_t	pending = 0;	/* pending seek if sparse */
+static off_t	last_sp = 0;	/* size of last added sparse block */
 u_int	ddflags = 0;		/* conversion options */
 size_t	cbsz;			/* conversion block size */
 uintmax_t files_cnt = 1;	/* # of files to copy */
@@ -173,6 +175,8 @@ setup(void)
 	} else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL ||
 	    (out.db = malloc(out.dbsz + cbsz)) == NULL)
 		err(1, "output buffer");
+
+	/* dbp is the first free position in each buffer. */
 	in.dbp = in.db;
 	out.dbp = out.db;
 
@@ -434,8 +438,15 @@ dd_out(int force)
 	 * we play games with the buffer size, and it's usually a partial write.
 	 */
 	outp = out.db;
+
+	/*
+	 * If force, first try to write all pending data, else try to write
+	 * just one block. Subsequently always write data one full block at
+	 * a time at most.
+	 */
 	for (n = force ? out.dbcnt : out.dbsz;; n = out.dbsz) {
-		for (cnt = n;; cnt -= nw) {
+		cnt = n;
+		do {
 			sparse = 0;
 			if (ddflags & C_SPARSE) {
 				sparse = 1;	/* Is buffer sparse? */
@@ -447,18 +458,24 @@ dd_out(int force)
 			}
 			if (sparse && !force) {
 				pending += cnt;
+				last_sp = cnt;
 				nw = cnt;
 			} else {
 				if (pending != 0) {
-					if (force)
-						pending--;
+					/* If forced to write, and we have no
+					 * data left, we need to write the last
+					 * sparse block explicitly.
+					 */
+					if (force && cnt == 0) {
+						pending -= last_sp;
+						assert(outp == out.db);
+						memset(outp, 0, cnt);
+					}
 					if (lseek(out.fd, pending, SEEK_CUR) ==
 					    -1)
 						err(2, "%s: seek error creating sparse file",
 						    out.name);
-					if (force)
-						write(out.fd, outp, 1);
-					pending = 0;
+					pending = last_sp = 0;
 				}
 				if (cnt)
 					nw = write(out.fd, outp, cnt);
@@ -473,27 +490,29 @@ dd_out(int force)
 					err(1, "%s", out.name);
 				nw = 0;
 			}
+
 			outp += nw;
 			st.bytes += nw;
-			if ((size_t)nw == n) {
-				if (n != out.dbsz)
-					++st.out_part;
-				else
-					++st.out_full;
-				break;
-			}
-			++st.out_part;
-			if ((size_t)nw == cnt)
-				break;
-			if (out.flags & ISTAPE)
-				errx(1, "%s: short write on tape device",
-				    out.name);
-			if (out.flags & ISCHR && !warned) {
-				warned = 1;
-				warnx("%s: short write on character device",
-				    out.name);
+
+			if ((size_t)nw == n && n == out.dbsz)
+				++st.out_full;
+			else
+				++st.out_part;
+
+			if ((size_t) nw != cnt) {
+				if (out.flags & ISTAPE)
+					errx(1, "%s: short write on tape device",
+				    	out.name);
+				if (out.flags & ISCHR && !warned) {
+					warned = 1;
+					warnx("%s: short write on character device",
+				    	out.name);
+				}
 			}
-		}
+
+			cnt -= nw;
+		} while (cnt != 0);
+
 		if ((out.dbcnt -= n) < out.dbsz)
 			break;
 	}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 6 Robert Schulze 2016-08-30 12:31:01 UTC
This patch breaks sparse file creation entirely.

I'm running 10.3-p6 and creating a sparse image for md-usage with dd is not possible, md cannot attach it. A sparse file created with truncate works.

I've reverted to r266488 and it works again.
Here is the output on non-reverted 10.3:

# dd if=/dev/zero of=disk bs=1M count=512 conv=sparse
512+0 records in
512+0 records out
536870912 bytes transferred in 0.255875 secs (2098175593 bytes/sec)
# ls -lsh
0 -rw-r--r--  1 root  wheel     0B Aug 30 14:29 disk

After reverting I got this:

# dd if=/dev/zero of=disk bs=1M count=512 conv=sparse
512+0 records in
512+0 records out
536870912 bytes transferred in 0.458561 secs (1170773306 bytes/sec)
# ls -lsh
96 -rw-r--r--  1 root  wheel   512M Aug 30 14:27 disk