{"thread":{"id":"26146","subject":"[RFC/PATCH 0/4] improving svn-fe error handling","startedAt":"2010-12-28T10:45:03Z","lastAt":"2011-01-04T10:16:05Z","messageCount":6,"participants":["Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"158633","messageId":"20101228104503.GA5422@burratino","threadId":"26146","inReplyTo":null,"subject":"[RFC/PATCH 0/4] improving svn-fe error handling","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-28T10:45:03Z","receivedAt":"2010-12-28T10:45:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nCurrently malformed streams (e.g., ending early) and input errors when\nreading a dump file can cause svn-fe to dereference a NULL pointer or\notherwise act in a confusing way.  Especially since incremental\nimports work now, I think a saner behavior is to error out with a\nclear error message; the operator can then fix the problem and resume.\nThis series is an attempt in that direction.\n\nStill to do:\n - add tests (reading /dev/full?  how else to provoke an input error?)\n - change buffer_read_string to disallow short reads, to simplify\n\nI am sending it out now because patches 1, 2, and 3 make API changes\nthat I find convenient, so I'd be interested in your thoughts before\nI commit too heavily to a bad idea.\n\nPatch 1 introduces buffer_ferror, to check if errors were encountered\nreading from the line_buffer.  Unlike buffer_deinit, this can be used\nafter any operation so it can be used early, when errno is still valid.\n\nPatches 2 and 3 make operations that can be partially completed\nstill return some indication of success or failure.\n\nPatch 4 is an example application, using the new API to make svn-fe\na little better at detecting malformed dump files.\n\nThoughts?\nJonathan Nieder (4):\n  vcs-svn: allow input errors to be detected promptly\n  vcs-svn: make buffer_skip_bytes return length read\n  vcs-svn: make buffer_copy_bytes return length read\n  vcs-svn: improve reporting of input errors\n\n vcs-svn/fast_export.c   |   13 +++++++++-\n vcs-svn/line_buffer.c   |   36 ++++++++++++++++++-------------\n vcs-svn/line_buffer.h   |    6 +++-\n vcs-svn/line_buffer.txt |    3 +-\n vcs-svn/svndump.c       |   53 +++++++++++++++++++++++++++++++++++++---------\n 5 files changed, 80 insertions(+), 31 deletions(-)\n"},{"id":"158634","messageId":"20101228104740.GA13360@burratino","threadId":"26146","inReplyTo":"20101228104503.GA5422@burratino","subject":"[PATCH 1/4] vcs-svn: allow input errors to be detected promptly","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-28T10:47:40Z","receivedAt":"2010-12-28T10:47:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Date: Sun, 10 Oct 2010 21:51:21 -0500\n\nThe line_buffer library silently flags input errors until\nbuffer_deinit time; unfortunately, by that point usually errno is\ninvalid.  Expose the error flag so callers can check for and\nreport errors early for easy debugging.\n\n\tsome_error_prone_operation(...);\n\tif (buffer_ferror())\n\t\treturn error(\"input error: %s\", strerror(errno));\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nHas been in use for a while, but still I'd be interested to know\nif this is sane.\n\n vcs-svn/line_buffer.c |    5 +++++\n vcs-svn/line_buffer.h |    1 +\n 2 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c\nindex 1543567..1b5ac8a 100644\n--- a/vcs-svn/line_buffer.c\n+++ b/vcs-svn/line_buffer.c\n@@ -35,6 +35,11 @@ int buffer_deinit(void)\n \treturn err;\n }\n \n+int buffer_ferror(void)\n+{\n+\treturn ferror(infile);\n+}\n+\n /* Read a line without trailing newline. */\n char *buffer_read_line(void)\n {\ndiff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h\nindex 9c78ae1..5a19873 100644\n--- a/vcs-svn/line_buffer.h\n+++ b/vcs-svn/line_buffer.h\n@@ -3,6 +3,7 @@\n \n int buffer_init(const char *filename);\n int buffer_deinit(void);\n+int buffer_ferror(void);\n char *buffer_read_line(void);\n char *buffer_read_string(uint32_t len);\n void buffer_copy_bytes(uint32_t len);\n-- \n1.7.2.3.554.gc9b5c.dirty\n"},{"id":"158635","messageId":"20101228104821.GB13360@burratino","threadId":"26146","inReplyTo":"20101228104503.GA5422@burratino","subject":"[PATCH 2/4] vcs-svn: make buffer_skip_bytes return length read","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-28T10:48:21Z","receivedAt":"2010-12-28T10:48:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Date: Sun, 10 Oct 2010 21:44:21 -0500\n\nCurrently there is no way to detect when input ended if it ended\nearly during buffer_skip_bytes.  Tell the calling program how many\nbytes were actually skipped for easier debugging.\n\nExisting callers will still ignore early EOF.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n vcs-svn/line_buffer.c   |   13 +++++++------\n vcs-svn/line_buffer.h   |    2 +-\n vcs-svn/line_buffer.txt |    3 ++-\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c\nindex 1b5ac8a..58e076f 100644\n--- a/vcs-svn/line_buffer.c\n+++ b/vcs-svn/line_buffer.c\n@@ -86,14 +86,15 @@ void buffer_copy_bytes(uint32_t len)\n \t}\n }\n \n-void buffer_skip_bytes(uint32_t len)\n+uint32_t buffer_skip_bytes(uint32_t nbytes)\n {\n-\tuint32_t in;\n-\twhile (len > 0 && !feof(infile) && !ferror(infile)) {\n-\t\tin = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;\n-\t\tin = fread(byte_buffer, 1, in, infile);\n-\t\tlen -= in;\n+\tuint32_t done = 0;\n+\twhile (done < nbytes && !feof(infile) && !ferror(infile)) {\n+\t\tuint32_t len = nbytes - done;\n+\t\tuint32_t in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;\n+\t\tdone += fread(byte_buffer, 1, in, infile);\n \t}\n+\treturn done;\n }\n \n void buffer_reset(void)\ndiff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h\nindex 5a19873..b9dd929 100644\n--- a/vcs-svn/line_buffer.h\n+++ b/vcs-svn/line_buffer.h\n@@ -7,7 +7,7 @@ int buffer_ferror(void);\n char *buffer_read_line(void);\n char *buffer_read_string(uint32_t len);\n void buffer_copy_bytes(uint32_t len);\n-void buffer_skip_bytes(uint32_t len);\n+uint32_t buffer_skip_bytes(uint32_t len);\n void buffer_reset(void);\n \n #endif\ndiff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt\nindex 8906fb1..a08ad8a 100644\n--- a/vcs-svn/line_buffer.txt\n+++ b/vcs-svn/line_buffer.txt\n@@ -52,7 +52,8 @@ Functions\n \n `buffer_skip_bytes`::\n \tDiscards `len` bytes from the input stream (stopping early\n-\tif necessary because of an error or eof).\n+\tif necessary because of an error or eof).  Return value is\n+\tthe number of bytes successfully read.\n \n `buffer_reset`::\n \tDeallocates non-static buffers.\n-- \n1.7.2.3.554.gc9b5c.dirty\n"},{"id":"158636","messageId":"20101228104936.GC13360@burratino","threadId":"26146","inReplyTo":"20101228104503.GA5422@burratino","subject":"[PATCH 3/4] vcs-svn: make buffer_copy_bytes return length read","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-28T10:49:36Z","receivedAt":"2010-12-28T10:49:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Currently buffer_copy_bytes does not report to its caller whether\nit encountered an early end of file.\n\nAdd a return value representing the number of bytes read (but not\nthe number of bytes copied).  This way all three unusual conditions\ncan be distinguished: input error with buffer_ferror, output error\nwith ferror(outfile), input error or early end of input by checking\nthe return value.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nIntuitive?\n\n vcs-svn/line_buffer.c |   18 +++++++++---------\n vcs-svn/line_buffer.h |    3 ++-\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c\nindex 58e076f..36f177c 100644\n--- a/vcs-svn/line_buffer.c\n+++ b/vcs-svn/line_buffer.c\n@@ -71,19 +71,19 @@ char *buffer_read_string(uint32_t len)\n \treturn ferror(infile) ? NULL : s;\n }\n \n-void buffer_copy_bytes(uint32_t len)\n+uint32_t buffer_copy_bytes(uint32_t nbytes)\n {\n-\tuint32_t in;\n-\twhile (len > 0 && !feof(infile) && !ferror(infile)) {\n-\t\tin = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;\n+\tuint32_t done = 0;\n+\twhile (done < nbytes && !feof(infile) && !ferror(infile)) {\n+\t\tuint32_t len = nbytes - done;\n+\t\tuint32_t in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;\n \t\tin = fread(byte_buffer, 1, in, infile);\n-\t\tlen -= in;\n+\t\tdone += in;\n \t\tfwrite(byte_buffer, 1, in, stdout);\n-\t\tif (ferror(stdout)) {\n-\t\t\tbuffer_skip_bytes(len);\n-\t\t\treturn;\n-\t\t}\n+\t\tif (ferror(stdout))\n+\t\t\treturn done + buffer_skip_bytes(nbytes - done);\n \t}\n+\treturn done;\n }\n \n uint32_t buffer_skip_bytes(uint32_t nbytes)\ndiff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h\nindex b9dd929..7766636 100644\n--- a/vcs-svn/line_buffer.h\n+++ b/vcs-svn/line_buffer.h\n@@ -6,7 +6,8 @@ int buffer_deinit(void);\n int buffer_ferror(void);\n char *buffer_read_line(void);\n char *buffer_read_string(uint32_t len);\n-void buffer_copy_bytes(uint32_t len);\n+/* Returns number of bytes read (not necessarily written). */\n+uint32_t buffer_copy_bytes(uint32_t len);\n uint32_t buffer_skip_bytes(uint32_t len);\n void buffer_reset(void);\n \n-- \n1.7.2.3.554.gc9b5c.dirty\n"},{"id":"158637","messageId":"20101228105410.GD13360@burratino","threadId":"26146","inReplyTo":"20101228104503.GA5422@burratino","subject":"[PATCH 4/4] vcs-svn: improve reporting of input errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-12-28T10:54:10Z","receivedAt":"2010-12-28T10:54:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Catch input errors and exit early enough to print a reasonable\ndiagnosis based on errno.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nPatches 1 and 2 first appeared in the svndiff0 series[1] (and that\nis the application I have in mind in sending them now).\n\nThis patch #4 is just a demo.  Untested, I'm afraid.  I would be\nvery interested in ideas for automatically testing it --- how to\neasily stimulate error paths?\n\nGood night,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/158731\n\n vcs-svn/fast_export.c |   13 ++++++++++-\n vcs-svn/svndump.c     |   53 ++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 256a052..dc0ae4e 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -62,14 +62,23 @@ void fast_export_commit(uint32_t revision, uint32_t author, char *log,\n \tprintf(\"progress Imported commit %d.\\n\\n\", revision);\n }\n \n+static void die_short_read(void)\n+{\n+\tif (buffer_ferror())\n+\t\tdie_errno(\"error reading dump file\");\n+\tdie(\"invalid dump: unexpected end of file\");\n+}\n+\n void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len)\n {\n \tif (mode == REPO_MODE_LNK) {\n \t\t/* svn symlink blobs start with \"link \" */\n-\t\tbuffer_skip_bytes(5);\n \t\tlen -= 5;\n+\t\tif (buffer_skip_bytes(5) != 5)\n+\t\t\tdie_short_read();\n \t}\n \tprintf(\"blob\\nmark :%d\\ndata %d\\n\", mark, len);\n-\tbuffer_copy_bytes(len);\n+\tif (buffer_copy_bytes(len) != len)\n+\t\tdie_short_read();\n \tfputc('\\n', stdout);\n }\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex 630eeb5..09bcebd 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -107,20 +107,37 @@ static void init_keys(void)\n \tkeys.content_length = pool_intern(\"Content-length\");\n }\n \n+static void die_short_read(void)\n+{\n+\tif (buffer_ferror())\n+\t\tdie_errno(\"error reading dump file\");\n+\tdie(\"invalid dump: unexpected end of file\");\n+}\n+\n static void read_props(void)\n {\n \tuint32_t len;\n \tuint32_t key = ~0;\n \tchar *val = NULL;\n-\tchar *t;\n-\twhile ((t = buffer_read_line()) && strcmp(t, \"PROPS-END\")) {\n+\tfor (;;) {\n+\t\tchar *t = buffer_read_line();\n+\t\tif (!t)\n+\t\t\tdie_short_read();\n+\t\tif (!strcmp(t, \"PROPS-END\"))\n+\t\t\treturn;\n \t\tif (!strncmp(t, \"K \", 2)) {\n \t\t\tlen = atoi(&t[2]);\n-\t\t\tkey = pool_intern(buffer_read_string(len));\n-\t\t\tbuffer_read_line();\n+\t\t\tt = buffer_read_line();\n+\t\t\tif (!t)\n+\t\t\t\tdie_short_read();\n+\t\t\tif (strlen(t) != len)\n+\t\t\t\tdie(\"invalid dump: incorrect key length\");\n+\t\t\tkey = pool_intern(t);\n \t\t} else if (!strncmp(t, \"V \", 2)) {\n \t\t\tlen = atoi(&t[2]);\n \t\t\tval = buffer_read_string(len);\n+\t\t\tif (!t)\n+\t\t\t\tdie_short_read();\n \t\t\tif (key == keys.svn_log) {\n \t\t\t\t/* Value length excludes terminating nul. */\n \t\t\t\trev_ctx.log = log_copy(len + 1, val);\n@@ -135,7 +152,11 @@ static void read_props(void)\n \t\t\t\tnode_ctx.type = REPO_MODE_LNK;\n \t\t\t}\n \t\t\tkey = ~0;\n-\t\t\tbuffer_read_line();\n+\t\t\tt = buffer_read_line();\n+\t\t\tif (!t)\n+\t\t\t\tdie_short_read();\n+\t\t\tif (*t)\n+\t\t\t\tdie(\"invalid dump: incorrect value length\");\n \t\t}\n \t}\n }\n@@ -176,10 +197,12 @@ static void handle_node(void)\n \tif (node_ctx.propLength == LENGTH_UNKNOWN && node_ctx.srcMode)\n \t\tnode_ctx.type = node_ctx.srcMode;\n \n-\tif (node_ctx.mark)\n+\tif (node_ctx.mark) {\n \t\tfast_export_blob(node_ctx.type, node_ctx.mark, node_ctx.textLength);\n-\telse if (node_ctx.textLength != LENGTH_UNKNOWN)\n-\t\tbuffer_skip_bytes(node_ctx.textLength);\n+\t} else if (node_ctx.textLength != LENGTH_UNKNOWN) {\n+\t\tif (buffer_skip_bytes(node_ctx.textLength) != node_ctx.textLength)\n+\t\t\tdie_short_read();\n+\t}\n }\n \n static void handle_revision(void)\n@@ -250,7 +273,11 @@ void svndump_read(const char *url)\n \t\t\tnode_ctx.propLength = atoi(val);\n \t\t} else if (key == keys.content_length) {\n \t\t\tlen = atoi(val);\n-\t\t\tbuffer_read_line();\n+\t\t\tt = buffer_read_line();\n+\t\t\tif (!t)\n+\t\t\t\tdie_short_read();\n+\t\t\tif (*t)\n+\t\t\t\tdie(\"invalid dump: expected blank line after content length header\");\n \t\t\tif (active_ctx == REV_CTX) {\n \t\t\t\tread_props();\n \t\t\t} else if (active_ctx == NODE_CTX) {\n@@ -258,10 +285,13 @@ void svndump_read(const char *url)\n \t\t\t\tactive_ctx = REV_CTX;\n \t\t\t} else {\n \t\t\t\tfprintf(stderr, \"Unexpected content length header: %d\\n\", len);\n-\t\t\t\tbuffer_skip_bytes(len);\n+\t\t\t\tif (buffer_skip_bytes(len) != len)\n+\t\t\t\t\tdie_short_read();\n \t\t\t}\n \t\t}\n \t}\n+\tif (buffer_ferror())\n+\t\tdie_short_read();\n \tif (active_ctx == NODE_CTX)\n \t\thandle_node();\n \tif (active_ctx != DUMP_CTX)\n@@ -270,7 +300,8 @@ void svndump_read(const char *url)\n \n void svndump_init(const char *filename)\n {\n-\tbuffer_init(filename);\n+\tif (buffer_init(filename))\n+\t\tdie_errno(\"cannot open dump file: %s\", filename);\n \trepo_init();\n \treset_dump_ctx(~0);\n \treset_rev_ctx(0);\n-- \n1.7.2.3.554.gc9b5c.dirty\n"},{"id":"158878","messageId":"20110104101605.GA24844@burratino","threadId":"26146","inReplyTo":"20101228105410.GD13360@burratino","subject":"Re: [PATCH 4/4] vcs-svn: improve reporting of input errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-01-04T10:16:05Z","receivedAt":"2011-01-04T10:16:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> --- a/vcs-svn/svndump.c\n> +++ b/vcs-svn/svndump.c\n> @@ -107,20 +107,37 @@ static void init_keys(void)\n[...]\n>  \t\t} else if (!strncmp(t, \"V \", 2)) {\n>  \t\t\tlen = atoi(&t[2]);\n>  \t\t\tval = buffer_read_string(len);\n> +\t\t\tif (!t)\n> +\t\t\t\tdie_short_read();\n\nThis should have said \"if (!val)\".  Sorry for the confusion.\n"}]}