{"thread":{"id":"27481","subject":"[PATCH/RFC db/text-delta 0/4] vcs-svn: avoid hangs for corrupt deltas","startedAt":"2011-05-27T11:08:28Z","lastAt":"2011-05-31T16:32:59Z","messageCount":7,"participants":["Jonathan Nieder","Drew Northup"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"168858","messageId":"20110527110828.GA7972@elie","threadId":"27481","inReplyTo":"BANLkTinBGnCKsUOXY_RD-7yNyM7XqNTsRw@mail.gmail.com","subject":"[PATCH/RFC db/text-delta 0/4] vcs-svn: avoid hangs for corrupt deltas","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-27T11:08:28Z","receivedAt":"2011-05-27T11:08:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nAs promised, here's a quick series to stop a too-greedy delta from\ncausing svn-fe deadlock.  It took longer than I thought it would,\nmostly because I started too late.\n\nIf I remember correctly, this first showed up as a theoretical\npossibility when chatting with Ram and later showed up in practice due\nto a bug in a test script or something like that.  I don't remember\nthe details but luckily it is not hard to make up a delta exhibiting\nthe problem.  See the test script in patch 4 for details.\n\nAfter review, I'd like to push this to the main svn-fe branch and ask\nJunio to pull it so text delta support can be rolled out.  There's\njust no reason not to, except for the obvious ones:\n\n - less pleasant raw UI for svn-fe\n - losing support for non-git fast-import backends that don't\n   support cat-blob yet\n\nWaiting for those would be to put the cart before the horse (how can\npeople experiment with making something better if they haven't\nexperienced what we have now?).  Review of the pending patches at\n\n git://repo.or.cz/git/jrn.git svn-fe-next\n\nto find missed details or other things that would be nice to get\ntaken care of toward that end would be very welcome.\n\nAs for this series: hopefully it's simple but it's perfectly\npossible I got something terribly wrong.  Thoughts welcome, as\nalways.\n\nThanks to David and Ram for some useful discussions on irc, including\nthose that led to these patches.\n\nJonathan Nieder (4):\n  test-svn-fe: split off \"test-svn-fe -d\" into a separate function\n  vcs-svn: cap number of bytes read from sliding view\n  vcs-svn: guard against overflow when computing preimage length\n  vcs-svn: avoid hangs from corrupt deltas\n"},{"id":"168859","messageId":"20110527110914.GB7972@elie","threadId":"27481","inReplyTo":"20110527110828.GA7972@elie","subject":"[PATCH 1/4] test-svn-fe: split off \"test-svn-fe -d\" into a separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-27T11:09:14Z","receivedAt":"2011-05-27T11:09:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The helper for testing the svndiff library is getting dangerously\nclose to the right margin.  Split it off into a separate function so\nit is easier to contemplate on its own.\n\nIn other words, this just unindents the code a little.  No functional\nchange intended.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n test-svn-fe.c |   54 ++++++++++++++++++++++++++++++++----------------------\n 1 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/test-svn-fe.c b/test-svn-fe.c\nindex 66bd040..a027626 100644\n--- a/test-svn-fe.c\n+++ b/test-svn-fe.c\n@@ -8,10 +8,37 @@\n #include \"vcs-svn/sliding_window.h\"\n #include \"vcs-svn/line_buffer.h\"\n \n+static const char test_svnfe_usage[] =\n+\t\"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)\";\n+\n+static int apply_delta(int argc, char *argv[])\n+{\n+\tstruct line_buffer preimage = LINE_BUFFER_INIT;\n+\tstruct line_buffer delta = LINE_BUFFER_INIT;\n+\tstruct sliding_view preimage_view = SLIDING_VIEW_INIT(&preimage);\n+\n+\tif (argc != 5)\n+\t\tusage(test_svnfe_usage);\n+\n+\tif (buffer_init(&preimage, argv[2]))\n+\t\tdie_errno(\"cannot open preimage\");\n+\tif (buffer_init(&delta, argv[3]))\n+\t\tdie_errno(\"cannot open delta\");\n+\tif (svndiff0_apply(&delta, (off_t) strtoull(argv[4], NULL, 0),\n+\t\t\t\t\t&preimage_view, stdout))\n+\t\treturn 1;\n+\tif (buffer_deinit(&preimage))\n+\t\tdie_errno(\"cannot close preimage\");\n+\tif (buffer_deinit(&delta))\n+\t\tdie_errno(\"cannot close delta\");\n+\tbuffer_reset(&preimage);\n+\tstrbuf_release(&preimage_view.buf);\n+\tbuffer_reset(&delta);\n+\treturn 0;\n+}\n+\n int main(int argc, char *argv[])\n {\n-\tstatic const char test_svnfe_usage[] =\n-\t\t\"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)\";\n \tif (argc == 2) {\n \t\tif (svndump_init(argv[1]))\n \t\t\treturn 1;\n@@ -20,25 +47,8 @@ int main(int argc, char *argv[])\n \t\tsvndump_reset();\n \t\treturn 0;\n \t}\n-\tif (argc == 5 && !strcmp(argv[1], \"-d\")) {\n-\t\tstruct line_buffer preimage = LINE_BUFFER_INIT;\n-\t\tstruct line_buffer delta = LINE_BUFFER_INIT;\n-\t\tstruct sliding_view preimage_view = SLIDING_VIEW_INIT(&preimage);\n-\t\tif (buffer_init(&preimage, argv[2]))\n-\t\t\tdie_errno(\"cannot open preimage\");\n-\t\tif (buffer_init(&delta, argv[3]))\n-\t\t\tdie_errno(\"cannot open delta\");\n-\t\tif (svndiff0_apply(&delta, (off_t) strtoull(argv[4], NULL, 0),\n-\t\t\t\t\t&preimage_view, stdout))\n-\t\t\treturn 1;\n-\t\tif (buffer_deinit(&preimage))\n-\t\t\tdie_errno(\"cannot close preimage\");\n-\t\tif (buffer_deinit(&delta))\n-\t\t\tdie_errno(\"cannot close delta\");\n-\t\tbuffer_reset(&preimage);\n-\t\tstrbuf_release(&preimage_view.buf);\n-\t\tbuffer_reset(&delta);\n-\t\treturn 0;\n-\t}\n+\n+\tif (argc >= 2 && !strcmp(argv[1], \"-d\"))\n+\t\treturn apply_delta(argc, argv);\n \tusage(test_svnfe_usage);\n }\n-- \n1.7.5.1\n"},{"id":"168860","messageId":"20110527111059.GC7972@elie","threadId":"27481","inReplyTo":"20110527110828.GA7972@elie","subject":"[PATCH 2/4] vcs-svn: cap number of bytes read from sliding view","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-27T11:11:00Z","receivedAt":"2011-05-27T11:11:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Introduce a \"max_off\" field in struct sliding_view, roughly speaking\nrepresenting a maximum number of bytes that can be read from \"file\".\nMore precisely, if it is set to a nonnegative integer, a call to\nmove_window() attempting to put the right endpoint beyond that offset\nwill return an error instead.  A value of -1 disables the check.\n\nThe idea is to use this when applying Subversion-format deltas to\nprevent reads past the end of the preimage (which has known length).\nWithout such a check, corrupt deltas would cause svn-fe to block\nindefinitely when data in the input pipe is exhausted.\n\nInspired-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n test-svn-fe.c            |    2 +-\n vcs-svn/sliding_window.c |    2 ++\n vcs-svn/sliding_window.h |    3 ++-\n 3 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/test-svn-fe.c b/test-svn-fe.c\nindex a027626..332a5f7 100644\n--- a/test-svn-fe.c\n+++ b/test-svn-fe.c\n@@ -15,7 +15,7 @@ static int apply_delta(int argc, char *argv[])\n {\n \tstruct line_buffer preimage = LINE_BUFFER_INIT;\n \tstruct line_buffer delta = LINE_BUFFER_INIT;\n-\tstruct sliding_view preimage_view = SLIDING_VIEW_INIT(&preimage);\n+\tstruct sliding_view preimage_view = SLIDING_VIEW_INIT(&preimage, -1);\n \n \tif (argc != 5)\n \t\tusage(test_svnfe_usage);\ndiff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\nindex 1b8d987..1bac7a4 100644\n--- a/vcs-svn/sliding_window.c\n+++ b/vcs-svn/sliding_window.c\n@@ -54,6 +54,8 @@ int move_window(struct sliding_view *view, off_t off, size_t width)\n \t\treturn -1;\n \tif (off < view->off || off + width < view->off + view->width)\n \t\treturn error(\"invalid delta: window slides left\");\n+\tif (view->max_off >= 0 && view->max_off < off + width)\n+\t\treturn error(\"delta preimage ends early\");\n \n \tfile_offset = view->off + view->buf.len;\n \tif (off < file_offset) {\ndiff --git a/vcs-svn/sliding_window.h b/vcs-svn/sliding_window.h\nindex ed0bfdd..b43a825 100644\n--- a/vcs-svn/sliding_window.h\n+++ b/vcs-svn/sliding_window.h\n@@ -7,10 +7,11 @@ struct sliding_view {\n \tstruct line_buffer *file;\n \toff_t off;\n \tsize_t width;\n+\toff_t max_off;\t/* -1 means unlimited */\n \tstruct strbuf buf;\n };\n \n-#define SLIDING_VIEW_INIT(input)\t{ (input), 0, 0, STRBUF_INIT }\n+#define SLIDING_VIEW_INIT(input, len)\t{ (input), 0, 0, (len), STRBUF_INIT }\n \n extern int move_window(struct sliding_view *view, off_t off, size_t width);\n \n-- \n1.7.5.1\n"},{"id":"168861","messageId":"20110527111233.GD7972@elie","threadId":"27481","inReplyTo":"20110527110828.GA7972@elie","subject":"[PATCH 3/4] vcs-svn: guard against overflow when computing preimage length","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-27T11:12:33Z","receivedAt":"2011-05-27T11:12:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Signed integer overflow produces undefined behavior in C and off_t is\na signed type.  For predictable behavior, add some checks to protect\nin advance against overflow.\n\nOn 32-bit systems, ftell as called by buffer_tmpfile_prepare_to_read\nis likely to fail with EOVERFLOW when reading the corresponding\npostimage, and this patch does not change that.  So this is more of a\nfutureproofing measure than a complete fix.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n vcs-svn/fast_export.c |   15 ++++++++++++++-\n 1 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex edc658d..96a75d5 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -166,6 +166,7 @@ static int ends_with(const char *s, size_t len, const char *suffix)\n static int parse_cat_response_line(const char *header, off_t *len)\n {\n \tsize_t headerlen = strlen(header);\n+\tuintmax_t n;\n \tconst char *type;\n \tconst char *end;\n \n@@ -174,14 +175,25 @@ static int parse_cat_response_line(const char *header, off_t *len)\n \ttype = memmem(header, headerlen, \" blob \", strlen(\" blob \"));\n \tif (!type)\n \t\treturn error(\"cat-blob header has wrong object type: %s\", header);\n-\t*len = strtoumax(type + strlen(\" blob \"), (char **) &end, 10);\n+\tn = strtoumax(type + strlen(\" blob \"), (char **) &end, 10);\n \tif (end == type + strlen(\" blob \"))\n \t\treturn error(\"cat-blob header does not contain length: %s\", header);\n+\tif (memchr(type + strlen(\" blob \"), '-', end - type - strlen(\" blob \")))\n+\t\treturn error(\"cat-blob header contains negative length: %s\", header);\n+\tif (n == UINTMAX_MAX || n > maximum_signed_value_of_type(off_t))\n+\t\treturn error(\"blob too large for current definition of off_t\");\n+\t*len = n;\n \tif (*end)\n \t\treturn error(\"cat-blob header contains garbage after length: %s\", header);\n \treturn 0;\n }\n \n+static void check_preimage_overflow(off_t a, off_t b)\n+{\n+\tif (signed_add_overflows(a, b))\n+\t\tdie(\"blob too large for current definition of off_t\");\n+}\n+\n static long apply_delta(off_t len, struct line_buffer *input,\n \t\t\tconst char *old_data, uint32_t old_mode)\n {\n@@ -204,6 +216,7 @@ static long apply_delta(off_t len, struct line_buffer *input,\n \t}\n \tif (old_mode == REPO_MODE_LNK) {\n \t\tstrbuf_addstr(&preimage.buf, \"link \");\n+\t\tcheck_preimage_overflow(preimage_len, strlen(\"link \"));\n \t\tpreimage_len += strlen(\"link \");\n \t}\n \tif (svndiff0_apply(input, len, &preimage, out))\n-- \n1.7.5.1\n"},{"id":"168862","messageId":"20110527111424.GE7972@elie","threadId":"27481","inReplyTo":"20110527110828.GA7972@elie","subject":"[PATCH 4/4] vcs-svn: avoid hangs from corrupt deltas","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-27T11:14:24Z","receivedAt":"2011-05-27T11:14:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"A corrupt Subversion-format delta can request reads past the end of\nthe preimage.  Set sliding_view::max_off so such corruption is caught\nwhen it appears rather than blocking in an impossible-to-fulfill\nread() when input is coming from a socket or pipe.\n\nInspired-by: Ramkumar Ramachandra <artagnon@gmail.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThe title feature.  Thanks for reading.\n\n t/t9010-svn-fe.sh     |   40 +++++++++++++++++++++++++++++++++++++---\n vcs-svn/fast_export.c |   15 +++++++++------\n 2 files changed, 46 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh\nindex f24f004..b7eed24 100755\n--- a/t/t9010-svn-fe.sh\n+++ b/t/t9010-svn-fe.sh\n@@ -18,12 +18,13 @@ reinit_git () {\n \n try_dump () {\n \tinput=$1 &&\n-\tmaybe_fail=${2:+test_$2} &&\n+\tmaybe_fail_svnfe=${2:+test_$2} &&\n+\tmaybe_fail_fi=${3:+test_$3} &&\n \n \t{\n-\t\t$maybe_fail test-svn-fe \"$input\" >stream 3<backflow &\n+\t\t$maybe_fail_svnfe test-svn-fe \"$input\" >stream 3<backflow &\n \t} &&\n-\tgit fast-import --cat-blob-fd=3 <stream 3>backflow &&\n+\t$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&\n \twait $!\n }\n \n@@ -1047,6 +1048,39 @@ test_expect_success PIPE 'deltas need not consume the whole preimage' '\n \ttest_cmp expect.3 actual.3\n '\n \n+test_expect_success PIPE 'no hang for delta trying to read past end of preimage' '\n+\treinit_git &&\n+\t{\n+\t\t# COPY 1\n+\t\tprintf \"SVNQ%b%b\" \"Q\\001\\001\\002Q\" \"\\001Q\" |\n+\t\tq_to_nul\n+\t} >greedy.delta &&\n+\t{\n+\t\tcat <<-\\EOF &&\n+\t\tSVN-fs-dump-format-version: 3\n+\n+\t\tRevision-number: 1\n+\t\tProp-content-length: 10\n+\t\tContent-length: 10\n+\n+\t\tPROPS-END\n+\n+\t\tNode-path: bootstrap\n+\t\tNode-kind: file\n+\t\tNode-action: add\n+\t\tText-delta: true\n+\t\tProp-content-length: 10\n+\t\tEOF\n+\t\techo Text-content-length: $(wc -c <greedy.delta) &&\n+\t\techo Content-length: $((10 + $(wc -c <greedy.delta))) &&\n+\t\techo &&\n+\t\techo PROPS-END &&\n+\t\tcat greedy.delta &&\n+\t\techo\n+\t} >greedydelta.dump &&\n+\ttry_dump greedydelta.dump must_fail might_fail\n+'\n+\n test_expect_success 'set up svn repo' '\n \tsvnconf=$PWD/svnconf &&\n \tmkdir -p \"$svnconf\" &&\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 96a75d5..97f5fdf 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -198,8 +198,7 @@ static long apply_delta(off_t len, struct line_buffer *input,\n \t\t\tconst char *old_data, uint32_t old_mode)\n {\n \tlong ret;\n-\toff_t preimage_len = 0;\n-\tstruct sliding_view preimage = SLIDING_VIEW_INIT(&report_buffer, -1);\n+\tstruct sliding_view preimage = SLIDING_VIEW_INIT(&report_buffer, 0);\n \tFILE *out;\n \n \tif (init_postimage() || !(out = buffer_tmpfile_rewind(&postimage)))\n@@ -211,19 +210,23 @@ static long apply_delta(off_t len, struct line_buffer *input,\n \t\tprintf(\"cat-blob %s\\n\", old_data);\n \t\tfflush(stdout);\n \t\tresponse = get_response_line();\n-\t\tif (parse_cat_response_line(response, &preimage_len))\n+\t\tif (parse_cat_response_line(response, &preimage.max_off))\n \t\t\tdie(\"invalid cat-blob response: %s\", response);\n+\t\tcheck_preimage_overflow(preimage.max_off, 1);\n \t}\n \tif (old_mode == REPO_MODE_LNK) {\n \t\tstrbuf_addstr(&preimage.buf, \"link \");\n-\t\tcheck_preimage_overflow(preimage_len, strlen(\"link \"));\n-\t\tpreimage_len += strlen(\"link \");\n+\t\tcheck_preimage_overflow(preimage.max_off, strlen(\"link \"));\n+\t\tpreimage.max_off += strlen(\"link \");\n+\t\tcheck_preimage_overflow(preimage.max_off, 1);\n \t}\n \tif (svndiff0_apply(input, len, &preimage, out))\n \t\tdie(\"cannot apply delta\");\n \tif (old_data) {\n \t\t/* Read the remainder of preimage and trailing newline. */\n-\t\tif (move_window(&preimage, preimage_len, 1))\n+\t\tassert(!signed_add_overflows(preimage.max_off, 1));\n+\t\tpreimage.max_off++;\t/* room for newline */\n+\t\tif (move_window(&preimage, preimage.max_off - 1, 1))\n \t\t\tdie(\"cannot seek to end of input\");\n \t\tif (preimage.buf.buf[0] != '\\n')\n \t\t\tdie(\"missing newline after cat-blob response\");\n-- \n1.7.5.1\n"},{"id":"169057","messageId":"1306858704.13283.2.camel@drew-northup.unet.maine.edu","threadId":"27481","inReplyTo":"20110527110914.GB7972@elie","subject":"Re: [PATCH 1/4] test-svn-fe: split off \"test-svn-fe -d\" into a separate function","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-05-31T16:18:24Z","receivedAt":"2011-05-31T16:18:24Z","isPatch":true,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Fri, 2011-05-27 at 06:09 -0500, Jonathan Nieder wrote:\n> The helper for testing the svndiff library is getting dangerously\n> close to the right margin.  Split it off into a separate function so\n> it is easier to contemplate on its own.\n> \n> In other words, this just unindents the code a little.  No functional\n> change intended.\n\n>  \n> +static const char test_svnfe_usage[] =\n> +\t\"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)\";\n\n>  int main(int argc, char *argv[])\n>  {\n> -\tstatic const char test_svnfe_usage[] =\n> -\t\t\"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)\";\n\nHow is this germane to the original intent? Is there a gain by making it\nglobal (to that file) that I'm missing?\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"169059","messageId":"20110531163259.GC390@elie","threadId":"27481","inReplyTo":"1306858704.13283.2.camel@drew-northup.unet.maine.edu","subject":"Re: [PATCH 1/4] test-svn-fe: split off \"test-svn-fe -d\" into a separate function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-31T16:32:59Z","receivedAt":"2011-05-31T16:32:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Drew Northup wrote:\n> On Fri, 2011-05-27 at 06:09 -0500, Jonathan Nieder wrote:\n\n>> +static const char test_svnfe_usage[] =\n>> +\t\"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)\";\n[...]\n> How is this germane to the original intent? Is there a gain by making it\n> global (to that file) that I'm missing?\n\nIt allows this variable with information about the program in general\nto be shared by the two functions.  Many examples of that style show\nup with\n\n\tgit grep -l -F -e 'main(' | xargs git grep -F -e 'usage[]' --\n\tgit grep -F -e 'usage[]' -- builtin\n\nI should have mentioned so in the commit message, though; maybe\nsomething like this:\n\n\tThe helper for testing the svndiff library is getting\n\tdangerously close to the right margin.  Split it off into a\n\tseparate function so it is easier to contemplate on its own.\n\n\tIn other words, this just unindents the code a little.  In the\n\tprocess, make the test_svnfe_usage[] string static so it can\n\tbe shared by the two functions (and other future functions in\n\tthis test program) without fuss.  No noticeable change\n\tintended.\n\nThanks for noticing,\nJonathan\n"}]}