login
Search
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(-) --
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
--
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 ...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 ...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) { ...
This should have said "if (!val)". Sorry for the confusion. --
