{"thread":{"id":"22578","subject":"[PATCH] blame: prevent a segv when -L given start > EOF","startedAt":"2010-02-09T03:48:13Z","lastAt":"2010-02-09T18:06:33Z","messageCount":11,"participants":["Jay Soffian","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"133988","messageId":"1265687293-11168-1-git-send-email-jaysoffian@gmail.com","threadId":"22578","inReplyTo":null,"subject":"[PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T03:48:13Z","receivedAt":"2010-02-09T03:48:13Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"blame would segv if given -L with past the end of the file.\nWhile we're fixing the bug, add test cases for an invalid when called\nas -L , or -L.\n\nSigned-off-by: Jay Soffian \n---\nbottom is start and top is end, which seems backwards to me, but alas, it is\nwhat it is. :-)\n\n builtin-blame.c | 2 +-\n t/t8003-blame.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 6408ec8..10f7eac 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2433,7 +2433,7 @@ parse_done:\n \tif (top < 1)\n \t\ttop = lno;\n \tbottom--;\n-\tif (lno < top)\n+\tif (lno < top || lno < bottom)\n \t\tdie(\"file %s has only %lu lines\", path, lno);\n \n \tent = xcalloc(1, sizeof(*ent));\ndiff --git a/t/t8003-blame.sh b/t/t8003-blame.sh\nindex ad834f2..4a8db74 100755\n--- a/t/t8003-blame.sh\n+++ b/t/t8003-blame.sh\n@@ -157,4 +157,12 @@ EOF\n git --no-pager blame $COMMIT -- uno >/dev/null\n '\n \n+test_expect_success 'blame -L with invalid start' '\n+\ttest_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n+'\n+\n+test_expect_success 'blame -L with invalid end' '\n+\tgit blame -L1,5 tres 2>&1 | grep \"has only 2 lines\"\n+'\n+\n test_done\n-- \n1.7.0.rc1.200.g9c1f9\n"},{"id":"133989","messageId":"76718491002081950g52b3dae7ta22377dfdd3479df@mail.gmail.com","threadId":"22578","inReplyTo":"1265687293-11168-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T03:50:11Z","receivedAt":"2010-02-09T03:50:11Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 8, 2010 at 10:48 PM, Jay Soffian wrote:\n> +test_expect_success 'blame -L with invalid start' '\n> +       test_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n> +'\n\nSorry, please remove test_must_fail after applying. Otherwise correct.\n\nj.\n"},{"id":"133990","messageId":"7vtytrrrju.fsf@alter.siamese.dyndns.org","threadId":"22578","inReplyTo":"1265687293-11168-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T03:55:49Z","receivedAt":"2010-02-09T03:55:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> -\tif (lno < top)\n> +\tif (lno < top || lno < bottom)\n> \t\tdie(\"file %s has only %lu lines\", path, lno);\n\nThanks; I think we make sure that \"bottom < top\" always hold true before\nwe reach this point, so checking with bottom alone should suffice, no?\n"},{"id":"133991","messageId":"76718491002082000s7be5b32dob5b3dfe764b12da0@mail.gmail.com","threadId":"22578","inReplyTo":"7vtytrrrju.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T04:00:14Z","receivedAt":"2010-02-09T04:00:14Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano wrote:\n> Jay Soffian writes:\n>\n>> -     if (lno < top)\n>> +     if (lno < top || lno < bottom)\n>>               die(\"file %s has only %lu lines\", path, lno);\n>\n> Thanks; I think we make sure that \"bottom < top\" always hold true before\n> we reach this point,\n\nWe swap them if they're reversed.\n\n> so checking with bottom alone should suffice, no?\n\nAh true. You can squash that in? :-)\n\nj.\n"},{"id":"133992","messageId":"76718491002082002n587c4b05x44e5f577cf17a8e1@mail.gmail.com","threadId":"22578","inReplyTo":"76718491002082000s7be5b32dob5b3dfe764b12da0@mail.gmail.com","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T04:02:46Z","receivedAt":"2010-02-09T04:02:46Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 8, 2010 at 11:00 PM, Jay Soffian wrote:\n>> so checking with bottom alone should suffice, no?\n>\n> Ah true. You can squash that in? :-)\n\nActually, no, that doesn't work, but my head can't keep bottom=start,\ntop=end straight so I'm not even sure why not.\n\nj.\n"},{"id":"133994","messageId":"76718491002082009u6a94d628ncf060820d427df7c@mail.gmail.com","threadId":"22578","inReplyTo":"7vtytrrrju.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T04:09:30Z","receivedAt":"2010-02-09T04:09:30Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Feb 8, 2010 at 10:55 PM, Junio C Hamano wrote:\n> Jay Soffian writes:\n>\n>> -     if (lno < top)\n>> +     if (lno < top || lno < bottom)\n>>               die(\"file %s has only %lu lines\", path, lno);\n>\n> Thanks; I think we make sure that \"bottom < top\" always hold true before\n> we reach this point, so checking with bottom alone should suffice, no?\n\nRight, so given \"-L 10\" at the point of that check:\n\nbottom = 0\ntop = 10\n\nWhereas given \"-L 10,100\" at the point of that check:\n\nbottom = 9\ntop = 100\n\nSo the code needs to check both. Previously it made sure neither was\n< 1, and swapped bottom/top if they were reversed.\n\nSorry for the confusion.\n\nj.\n"},{"id":"134002","messageId":"7vwrynq7fc.fsf@alter.siamese.dyndns.org","threadId":"22578","inReplyTo":"7vtytrrrju.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T05:55:51Z","receivedAt":"2010-02-09T05:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jay Soffian writes:\n>\n>> -\tif (lno < top)\n>> +\tif (lno < top || lno < bottom)\n>> \t\tdie(\"file %s has only %lu lines\", path, lno);\n>\n> Thanks; I think we make sure that \"bottom < top\" always hold true before\n> we reach this point, so checking with bottom alone should suffice, no?\n\nI am ... stupid. If lno < bottom, then lno < top, but we need to check\nboth anyway.\n"},{"id":"134013","messageId":"4B71122F.3040809@viscovery.net","threadId":"22578","inReplyTo":"1265687293-11168-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-09T07:43:43Z","receivedAt":"2010-02-09T07:43:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jay Soffian schrieb:\n> +test_expect_success 'blame -L with invalid start' '\n> +\ttest_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n\nPlease write this as\n\n\ttest_must_fail git blame -L5 tres >output 2>&1 &&\n\tgrep \"has only 2 lines\" output\n\n> +'\n> +\n> +test_expect_success 'blame -L with invalid end' '\n> +\tgit blame -L1,5 tres 2>&1 | grep \"has only 2 lines\"\n\n\ttest_must_fail git blame -L1,5 tres >output 2>&1 &&\n\tgrep \"has only 2 lines\" output\n\nbecause shells look only at the exit code of the last command in a pipeline.\n\n-- Hannes\n"},{"id":"134057","messageId":"76718491002090955g66eca03ar7438e57811616267@mail.gmail.com","threadId":"22578","inReplyTo":"4B71122F.3040809@viscovery.net","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T17:55:37Z","receivedAt":"2010-02-09T17:55:37Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Tue, Feb 9, 2010 at 2:43 AM, Johannes Sixt wrote:\n> Jay Soffian schrieb:\n>> +test_expect_success 'blame -L with invalid start' '\n>> +     test_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n>\n> Please write this as\n>\n>        test_must_fail git blame -L5 tres >output 2>&1 &&\n>        grep \"has only 2 lines\" output\n>\n>> +'\n>> +\n>> +test_expect_success 'blame -L with invalid end' '\n>> +     git blame -L1,5 tres 2>&1 | grep \"has only 2 lines\"\n>\n>        test_must_fail git blame -L1,5 tres >output 2>&1 &&\n>        grep \"has only 2 lines\" output\n>\n> because shells look only at the exit code of the last command in a pipeline.\n\nThanks, I knew that. I'd left in test_must_fail accidentally because\ninitially I wasn't bothering to grep the output. I then added the grep\nand forgot to remove test_must_fail. Isn't this an adequate test:\n\n test_expect_success 'blame -L with invalid start' '\n     git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n\nAs it seems unlikely git would crash and still output the message\ncorrectly in this case.\n\n?\n\nj.\n"},{"id":"134059","messageId":"1265738392-61503-1-git-send-email-jaysoffian@gmail.com","threadId":"22578","inReplyTo":"4B71122F.3040809@viscovery.net","subject":"[PATCH v2] blame: prevent a segv when -L given start > EOF","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-02-09T17:59:52Z","receivedAt":"2010-02-09T17:59:52Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"blame would segv if given -L with past the end of the file.\nWhile we're fixing the bug, add test cases for an invalid when called\nas -L , or -L.\n\nSigned-off-by: Jay Soffian \n---\nModified the tests per Hannes recommendations.\n\n builtin-blame.c | 2 +-\n t/t8003-blame.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 6408ec8..10f7eac 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -2433,7 +2433,7 @@ parse_done:\n \tif (top < 1)\n \t\ttop = lno;\n \tbottom--;\n-\tif (lno < top)\n+\tif (lno < top || lno < bottom)\n \t\tdie(\"file %s has only %lu lines\", path, lno);\n \n \tent = xcalloc(1, sizeof(*ent));\ndiff --git a/t/t8003-blame.sh b/t/t8003-blame.sh\nindex ad834f2..be648b2 100755\n--- a/t/t8003-blame.sh\n+++ b/t/t8003-blame.sh\n@@ -157,4 +157,14 @@ EOF\n git --no-pager blame $COMMIT -- uno >/dev/null\n '\n \n+test_expect_success 'blame -L with invalid start' '\n+\ttest_must_fail git blame -L5 tres >output 2>&1 &&\n+\tgrep \"has only 2 lines\" output\n+'\n+\n+test_expect_success 'blame -L with invalid end' '\n+\ttest_must_fail git blame -L1,5 tres >output 2>&1 &&\n+\tgrep \"has only 2 lines\" output\n+'\n+\n test_done\n-- \n1.7.0.rc1.200.g9c1f9\n"},{"id":"134061","messageId":"7vhbpqe11y.fsf@alter.siamese.dyndns.org","threadId":"22578","inReplyTo":"76718491002090955g66eca03ar7438e57811616267@mail.gmail.com","subject":"Re: [PATCH] blame: prevent a segv when -L given start > EOF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T18:06:33Z","receivedAt":"2010-02-09T18:06:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry for knowing but forgetting about the issue while applying.\nI'll queue this fix-up.\n\n-- >8 --\nt8003: check exit code of command and error message separately\n\nShell reports exit status only from the most downstream command\nin a pipeline. In these tests, we want to make sure that the\ncommand fails in a controlled way, and produces a correct error\nmessage.\n\nThis issue was known by Jay who submitted the patch, and also was\npointed out by Hannes during the review process, but I forgot to\nfix it up before applying. Sorry about that.\n\nSigned-off-by: Junio C Hamano \n---\n t/t8003-blame.sh | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t8003-blame.sh b/t/t8003-blame.sh\nindex 4a8db74..3bbddd0 100755\n--- a/t/t8003-blame.sh\n+++ b/t/t8003-blame.sh\n@@ -158,11 +158,13 @@ EOF\n '\n \n test_expect_success 'blame -L with invalid start' '\n-\ttest_must_fail git blame -L5 tres 2>&1 | grep \"has only 2 lines\"\n+\ttest_must_fail git blame -L5 tres 2>errors &&\n+\tgrep \"has only 2 lines\" errors\n '\n \n test_expect_success 'blame -L with invalid end' '\n-\tgit blame -L1,5 tres 2>&1 | grep \"has only 2 lines\"\n+\ttest_must_fail git blame -L1,5 tres 2>errors &&\n+\tgrep \"has only 2 lines\" errors\n '\n \n test_done\n"}]}