{"thread":{"id":"29283","subject":"[PATCH] fix hang in git fetch if pointed at a 0 length bundle","startedAt":"2012-01-03T01:13:52Z","lastAt":"2012-01-03T20:13:14Z","messageCount":6,"participants":["Brian Harring","Johannes Sixt","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"181851","messageId":"20120103011352.GA13825@localhost","threadId":"29283","inReplyTo":null,"subject":"[PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Brian Harring","fromEmail":"ferringb@gmail.com","sentAt":"2012-01-03T01:13:52Z","receivedAt":"2012-01-03T01:13:52Z","isPatch":true,"sender":{"key":"ferringb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1531180?v=4"},"body":"git-repo if interupted at the exact wrong time will generate zero\nlength bundles- literal empty files.  git-repo is wrong here, but\ngit fetch shouldn't effectively spin loop if pointed at a zero\nlength bundle.\n\nSigned-off-by: Brian Harring <ferringb@chromium.org>\n---\n bundle.c          |    2 +-\n t/t5704-bundle.sh |   10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 4742f27..ac9cc20 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)\n \twhile (1) {\n \t\tchar ch;\n \t\tssize_t len = xread(fd, &ch, 1);\n-\t\tif (len < 0)\n+\t\tif (len <= 0)\n \t\t\treturn -1;\n \t\tstrbuf_addch(sb, ch);\n \t\tif (ch == '\\n')\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 728ccd8..75ce5b8 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -53,4 +53,14 @@ test_expect_failure 'bundle --stdin <rev-list options>' '\n \n '\n \n+test_expect_success 'die if bundle file is empty' '\n+\n+   echo -n > empty-bundle\n+   timeout 5 git fetch empty-bundle\n+   ret=$?\n+   [ $ret == 128 ] && return 0\n+   return $ret\n+\n+'\n+\n test_done\n-- \n1.7.8.2\n"},{"id":"181854","messageId":"4F02BDEC.9060407@kdbg.org","threadId":"29283","inReplyTo":"20120103011352.GA13825@localhost","subject":"Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-01-03T08:35:56Z","receivedAt":"2012-01-03T08:35:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 03.01.2012 02:13, schrieb Brian Harring:\n> git-repo if interupted at the exact wrong time will generate zero\n> length bundles- literal empty files.  git-repo is wrong here, but\n> git fetch shouldn't effectively spin loop if pointed at a zero\n> length bundle.\n\nAdding a test case is very much appreciated.\n\n> +test_expect_success 'die if bundle file is empty' '\n\nHow about 'empty bundle file is rejected'?\n\n> +\n> +   echo -n > empty-bundle\n\n'echo -n' is not portable; use simply\n\n\t>empty-bundle &&\n\n(note the style: no blank after >). Also chain commands using &&.\n\n> +   timeout 5 git fetch empty-bundle\n\nYes, there was an infinite loop. But we do not specifically protect our\ngit invocations in the test suite against this sort of failure. Just write\n\n\ttest_must_fail git fetch empty-bundle\n\nand end the test case here.\n\n> +   ret=$?\n> +   [ $ret == 128 ] && return 0\n> +   return $ret\n> +\n> +'\n\nFurthermore, indentation should be one tabstop, not blanks.\n\n-- Hannes\n"},{"id":"181859","messageId":"CACsJy8B5B2bx7WK7ViziseuWCPaMgcc-avwtsw2DybRme8Vgfg@mail.gmail.com","threadId":"29283","inReplyTo":"20120103011352.GA13825@localhost","subject":"Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-01-03T12:07:53Z","receivedAt":"2012-01-03T12:07:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 3, 2012 at 8:13 AM, Brian Harring <ferringb@gmail.com> wrote:\n> @@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)\n>        while (1) {\n>                char ch;\n>                ssize_t len = xread(fd, &ch, 1);\n> -               if (len < 0)\n> +               if (len <= 0)\n>                        return -1;\n>                strbuf_addch(sb, ch);\n>                if (ch == '\\n')\n\nI think it should return 0 when len == 0 because strictly speaking eof\nis not a fault. It's not really important though because the two\ncallers in this file work fine either way.\n\nFWIW I went through all xread call sites. All seem to handle return\nvalue <= 0 correctly.\n-- \nDuy\n"},{"id":"181861","messageId":"20120103134603.GA31034@localhost","threadId":"29283","inReplyTo":"20120103011352.GA13825@localhost","subject":"[PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Brian Harring","fromEmail":"ferringb@gmail.com","sentAt":"2012-01-03T13:46:03Z","receivedAt":"2012-01-03T13:46:03Z","isPatch":true,"sender":{"key":"ferringb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1531180?v=4"},"body":"git-repo if interupted at the exact wrong time will generate zero\nlength bundles- literal empty files.  git-repo is wrong here, but\ngit fetch shouldn't effectively spin loop if pointed at a zero\nlength bundle.\n\nSigned-off-by: Brian Harring <ferringb@chromium.org>\n---\n bundle.c          |    4 ++--\n t/t5704-bundle.sh |    6 ++++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 4742f27..b8acf3c 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -31,8 +31,8 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)\n \twhile (1) {\n \t\tchar ch;\n \t\tssize_t len = xread(fd, &ch, 1);\n-\t\tif (len < 0)\n-\t\t\treturn -1;\n+\t\tif (len <= 0)\n+\t\t\treturn len;\n \t\tstrbuf_addch(sb, ch);\n \t\tif (ch == '\\n')\n \t\t\tbreak;\ndiff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh\nindex 728ccd8..4ae127d 100755\n--- a/t/t5704-bundle.sh\n+++ b/t/t5704-bundle.sh\n@@ -53,4 +53,10 @@ test_expect_failure 'bundle --stdin <rev-list options>' '\n \n '\n \n+test_expect_success 'empty bundle file is rejected' '\n+\n+    >empty-bundle && test_must_fail git fetch empty-bundle\n+\n+'\n+\n test_done\n-- \n1.7.8.2\n"},{"id":"181888","messageId":"7vboqktukz.fsf@alter.siamese.dyndns.org","threadId":"29283","inReplyTo":"CACsJy8B5B2bx7WK7ViziseuWCPaMgcc-avwtsw2DybRme8Vgfg@mail.gmail.com","subject":"Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-03T20:11:24Z","receivedAt":"2012-01-03T20:11:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Tue, Jan 3, 2012 at 8:13 AM, Brian Harring <ferringb@gmail.com> wrote:\n>> @@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)\n>>        while (1) {\n>>                char ch;\n>>                ssize_t len = xread(fd, &ch, 1);\n>> -               if (len < 0)\n>> +               if (len <= 0)\n>>                        return -1;\n>>                strbuf_addch(sb, ch);\n>>                if (ch == '\\n')\n>\n> I think it should return 0 when len == 0 because strictly speaking eof\n> is not a fault.\n\nEven if you do not strictly speak, end of file is a perfectly normal thing\nto see, no?  IOW wouldn't the original patch actively _break_ callers that\nread the whole file from the file descriptor to the end?\n\n> FWIW I went through all xread call sites. All seem to handle return\n> value <= 0 correctly.\n\nThanks.\n"},{"id":"181889","messageId":"7v7h18tuhx.fsf@alter.siamese.dyndns.org","threadId":"29283","inReplyTo":"20120103134603.GA31034@localhost","subject":"Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-03T20:13:14Z","receivedAt":"2012-01-03T20:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Harring <ferringb@gmail.com> writes:\n\n> git-repo if interupted at the exact wrong time will generate zero\n> length bundles- literal empty files.  git-repo is wrong here, but\n> git fetch shouldn't effectively spin loop if pointed at a zero\n> length bundle.\n>\n> Signed-off-by: Brian Harring <ferringb@chromium.org>\n> ---\n\nThanks.\n\nAlso thanks to all reviewers.\n"}]}