[RFC/PATCH 0/4] improving svn-fe error handling

[RFC/PATCH 0/4] improving svn-fe error handling

[Jonathan Nieder]

Hi,

Currently malformed streams (e.g., ending early) and input errors when
reading a dump file can cause svn-fe to dereference a NULL pointer or
otherwise act in a confusing way.  Especially since incremental
imports work now, I think a saner behavior is to error out with a
clear error message; the operator can then fix the problem and resume.
This series is an attempt in that direction.

Still to do:
 - add tests (reading /dev/full?  how else to provoke an input error?)
 - change buffer_read_string to disallow short reads, to simplify

I am sending it out now because patches 1, 2, and 3 make API changes
that I find convenient, so I'd be interested in your thoughts before
I commit too heavily to a bad idea.

Patch 1 introduces buffer_ferror, to check if errors were encountered
reading from the line_buffer.  Unlike buffer_deinit, this can be used
after any operation so it can be used early, when errno is still valid.

Patches 2 and 3 make operations that can be partially completed
still return some indication of success or failure.

Patch 4 is an example application, using the new API to make svn-fe
a little better at detecting malformed dump files.

Thoughts?
Jonathan Nieder (4):
  vcs-svn: allow input errors to be detected promptly
  vcs-svn: make buffer_skip_bytes return length read
  vcs-svn: make buffer_copy_bytes return length read
  vcs-svn: improve reporting of input errors

 vcs-svn/fast_export.c   |   13 +++++++++-
 vcs-svn/line_buffer.c   |   36 ++++++++++++++++++-------------
 vcs-svn/line_buffer.h   |    6 +++-
 vcs-svn/line_buffer.txt |    3 +-
 vcs-svn/svndump.c       |   53 +++++++++++++++++++++++++++++++++++++---------
 5 files changed, 80 insertions(+), 31 deletions(-)

[PATCH 1/4] vcs-svn: allow input errors to be detected promptly

[Jonathan Nieder]

Date: Sun, 10 Oct 2010 21:51:21 -0500

The line_buffer library silently flags input errors until
buffer_deinit time; unfortunately, by that point usually errno is
invalid.  Expose the error flag so callers can check for and
report errors early for easy debugging.

	some_error_prone_operation(...);
	if (buffer_ferror())
		return error("input error: %s", strerror(errno));

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Has been in use for a while, but still I'd be interested to know
if this is sane.

 vcs-svn/line_buffer.c |    5 +++++
 vcs-svn/line_buffer.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 1543567..1b5ac8a 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -35,6 +35,11 @@ int buffer_deinit(void)
 	return err;
 }
 
+int buffer_ferror(void)
+{
+	return ferror(infile);
+}
+
 /* Read a line without trailing newline. */
 char *buffer_read_line(void)
 {
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 9c78ae1..5a19873 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -3,6 +3,7 @@
 
 int buffer_init(const char *filename);
 int buffer_deinit(void);
+int buffer_ferror(void);
 char *buffer_read_line(void);
 char *buffer_read_string(uint32_t len);
 void buffer_copy_bytes(uint32_t len);
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 2/4] vcs-svn: make buffer_skip_bytes return length read

[Jonathan Nieder]

Date: Sun, 10 Oct 2010 21:44:21 -0500

Currently there is no way to detect when input ended if it ended
early during buffer_skip_bytes.  Tell the calling program how many
bytes were actually skipped for easier debugging.

Existing callers will still ignore early EOF.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 vcs-svn/line_buffer.c   |   13 +++++++------
 vcs-svn/line_buffer.h   |    2 +-
 vcs-svn/line_buffer.txt |    3 ++-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 1b5ac8a..58e076f 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -86,14 +86,15 @@ void buffer_copy_bytes(uint32_t len)
 	}
 }
 
-void buffer_skip_bytes(uint32_t len)
+uint32_t buffer_skip_bytes(uint32_t nbytes)
 {
-	uint32_t in;
-	while (len > 0 && !feof(infile) && !ferror(infile)) {
-		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
-		in = fread(byte_buffer, 1, in, infile);
-		len -= in;
+	uint32_t done = 0;
+	while (done < nbytes && !feof(infile) && !ferror(infile)) {
+		uint32_t len = nbytes - done;
+		uint32_t in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
+		done += fread(byte_buffer, 1, in, infile);
 	}
+	return done;
 }
 
 void buffer_reset(void)
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 5a19873..b9dd929 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -7,7 +7,7 @@ int buffer_ferror(void);
 char *buffer_read_line(void);
 char *buffer_read_string(uint32_t len);
 void buffer_copy_bytes(uint32_t len);
-void buffer_skip_bytes(uint32_t len);
+uint32_t buffer_skip_bytes(uint32_t len);
 void buffer_reset(void);
 
 #endif
diff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt
index 8906fb1..a08ad8a 100644
--- a/vcs-svn/line_buffer.txt
+++ b/vcs-svn/line_buffer.txt
@@ -52,7 +52,8 @@ Functions
 
 `buffer_skip_bytes`::
 	Discards `len` bytes from the input stream (stopping early
-	if necessary because of an error or eof).
+	if necessary because of an error or eof).  Return value is
+	the number of bytes successfully read.
 
 `buffer_reset`::
 	Deallocates non-static buffers.
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 3/4] vcs-svn: make buffer_copy_bytes return length read

[Jonathan Nieder]

Currently buffer_copy_bytes does not report to its caller whether
it encountered an early end of file.

Add a return value representing the number of bytes read (but not
the number of bytes copied).  This way all three unusual conditions
can be distinguished: input error with buffer_ferror, output error
with ferror(outfile), input error or early end of input by checking
the return value.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Intuitive?

 vcs-svn/line_buffer.c |   18 +++++++++---------
 vcs-svn/line_buffer.h |    3 ++-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 58e076f..36f177c 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -71,19 +71,19 @@ char *buffer_read_string(uint32_t len)
 	return ferror(infile) ? NULL : s;
 }
 
-void buffer_copy_bytes(uint32_t len)
+uint32_t buffer_copy_bytes(uint32_t nbytes)
 {
-	uint32_t in;
-	while (len > 0 && !feof(infile) && !ferror(infile)) {
-		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
+	uint32_t done = 0;
+	while (done < nbytes && !feof(infile) && !ferror(infile)) {
+		uint32_t len = nbytes - done;
+		uint32_t in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
 		in = fread(byte_buffer, 1, in, infile);
-		len -= in;
+		done += in;
 		fwrite(byte_buffer, 1, in, stdout);
-		if (ferror(stdout)) {
-			buffer_skip_bytes(len);
-			return;
-		}
+		if (ferror(stdout))
+			return done + buffer_skip_bytes(nbytes - done);
 	}
+	return done;
 }
 
 uint32_t buffer_skip_bytes(uint32_t nbytes)
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index b9dd929..7766636 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -6,7 +6,8 @@ int buffer_deinit(void);
 int buffer_ferror(void);
 char *buffer_read_line(void);
 char *buffer_read_string(uint32_t len);
-void buffer_copy_bytes(uint32_t len);
+/* Returns number of bytes read (not necessarily written). */
+uint32_t buffer_copy_bytes(uint32_t len);
 uint32_t buffer_skip_bytes(uint32_t len);
 void buffer_reset(void);
 
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 4/4] vcs-svn: improve reporting of input errors

[Jonathan Nieder]

Catch input errors and exit early enough to print a reasonable
diagnosis based on errno.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Patches 1 and 2 first appeared in the svndiff0 series[1] (and that
is the application I have in mind in sending them now).

This patch #4 is just a demo.  Untested, I'm afraid.  I would be
very interested in ideas for automatically testing it --- how to
easily stimulate error paths?

Good night,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/158731

 vcs-svn/fast_export.c |   13 ++++++++++-
 vcs-svn/svndump.c     |   53 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 256a052..dc0ae4e 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -62,14 +62,23 @@ void fast_export_commit(uint32_t revision, uint32_t author, char *log,
 	printf("progress Imported commit %d.\n\n", revision);
 }
 
+static void die_short_read(void)
+{
+	if (buffer_ferror())
+		die_errno("error reading dump file");
+	die("invalid dump: unexpected end of file");
+}
+
 void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len)
 {
 	if (mode == REPO_MODE_LNK) {
 		/* svn symlink blobs start with "link " */
-		buffer_skip_bytes(5);
 		len -= 5;
+		if (buffer_skip_bytes(5) != 5)
+			die_short_read();
 	}
 	printf("blob\nmark :%d\ndata %d\n", mark, len);
-	buffer_copy_bytes(len);
+	if (buffer_copy_bytes(len) != len)
+		die_short_read();
 	fputc('\n', stdout);
 }
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 630eeb5..09bcebd 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -107,20 +107,37 @@ static void init_keys(void)
 	keys.content_length = pool_intern("Content-length");
 }
 
+static void die_short_read(void)
+{
+	if (buffer_ferror())
+		die_errno("error reading dump file");
+	die("invalid dump: unexpected end of file");
+}
+
 static void read_props(void)
 {
 	uint32_t len;
 	uint32_t key = ~0;
 	char *val = NULL;
-	char *t;
-	while ((t = buffer_read_line()) && strcmp(t, "PROPS-END")) {
+	for (;;) {
+		char *t = buffer_read_line();
+		if (!t)
+			die_short_read();
+		if (!strcmp(t, "PROPS-END"))
+			return;
 		if (!strncmp(t, "K ", 2)) {
 			len = atoi(&t[2]);
-			key = pool_intern(buffer_read_string(len));
-			buffer_read_line();
+			t = buffer_read_line();
+			if (!t)
+				die_short_read();
+			if (strlen(t) != len)
+				die("invalid dump: incorrect key length");
+			key = pool_intern(t);
 		} else if (!strncmp(t, "V ", 2)) {
 			len = atoi(&t[2]);
 			val = buffer_read_string(len);
+			if (!t)
+				die_short_read();
 			if (key == keys.svn_log) {
 				/* Value length excludes terminating nul. */
 				rev_ctx.log = log_copy(len + 1, val);
@@ -135,7 +152,11 @@ static void read_props(void)
 				node_ctx.type = REPO_MODE_LNK;
 			}
 			key = ~0;
-			buffer_read_line();
+			t = buffer_read_line();
+			if (!t)
+				die_short_read();
+			if (*t)
+				die("invalid dump: incorrect value length");
 		}
 	}
 }
@@ -176,10 +197,12 @@ static void handle_node(void)
 	if (node_ctx.propLength == LENGTH_UNKNOWN && node_ctx.srcMode)
 		node_ctx.type = node_ctx.srcMode;
 
-	if (node_ctx.mark)
+	if (node_ctx.mark) {
 		fast_export_blob(node_ctx.type, node_ctx.mark, node_ctx.textLength);
-	else if (node_ctx.textLength != LENGTH_UNKNOWN)
-		buffer_skip_bytes(node_ctx.textLength);
+	} else if (node_ctx.textLength != LENGTH_UNKNOWN) {
+		if (buffer_skip_bytes(node_ctx.textLength) != node_ctx.textLength)
+			die_short_read();
+	}
 }
 
 static void handle_revision(void)
@@ -250,7 +273,11 @@ void svndump_read(const char *url)
 			node_ctx.propLength = atoi(val);
 		} else if (key == keys.content_length) {
 			len = atoi(val);
-			buffer_read_line();
+			t = buffer_read_line();
+			if (!t)
+				die_short_read();
+			if (*t)
+				die("invalid dump: expected blank line after content length header");
 			if (active_ctx == REV_CTX) {
 				read_props();
 			} else if (active_ctx == NODE_CTX) {
@@ -258,10 +285,13 @@ void svndump_read(const char *url)
 				active_ctx = REV_CTX;
 			} else {
 				fprintf(stderr, "Unexpected content length header: %d\n", len);
-				buffer_skip_bytes(len);
+				if (buffer_skip_bytes(len) != len)
+					die_short_read();
 			}
 		}
 	}
+	if (buffer_ferror())
+		die_short_read();
 	if (active_ctx == NODE_CTX)
 		handle_node();
 	if (active_ctx != DUMP_CTX)
@@ -270,7 +300,8 @@ void svndump_read(const char *url)
 
 void svndump_init(const char *filename)
 {
-	buffer_init(filename);
+	if (buffer_init(filename))
+		die_errno("cannot open dump file: %s", filename);
 	repo_init();
 	reset_dump_ctx(~0);
 	reset_rev_ctx(0);
-- 
1.7.2.3.554.gc9b5c.dirty

Re: [PATCH 4/4] vcs-svn: improve reporting of input errors

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -107,20 +107,37 @@ static void init_keys(void)
[...]
>  		} else if (!strncmp(t, "V ", 2)) {
>  			len = atoi(&t[2]);
>  			val = buffer_read_string(len);
> +			if (!t)
> +				die_short_read();

This should have said "if (!val)".  Sorry for the confusion.