{"thread":{"id":"7198","subject":"\"git-diff -p :/anything\" always segfaults","startedAt":"2007-03-11T18:49:08Z","lastAt":"2007-03-12T18:42:11Z","messageCount":7,"participants":["Jim Meyering","Johannes Schindelin","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"36841","messageId":"87d53fsjiz.fsf@rho.meyering.net","threadId":"7198","inReplyTo":null,"subject":"\"git-diff -p :/anything\" always segfaults","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2007-03-11T18:49:08Z","receivedAt":"2007-03-11T18:49:08Z","isPatch":false,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"I like the idea of the new ':/' notation, and gave it\na try, but all I could get was a segfault. It was dereferencing a NULL\ncommit list. Fix below. With it, this example now works:\n\n$ mkdir .j; cd .j; touch f\n$ (git-init; git-add f; git-commit -mc f; echo x >f; git-commit -md f)>/dev/null\n$ git-diff -p :/c :/d\ndiff --git a/f b/f\nindex e69de29..587be6b 100644\n--- a/f\n+++ b/f\n@@ -0,0 +1 @@\n+x\n\nSigned-off-by: Jim Meyering \n---\n sha1_name.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 31812d3..6b6270b 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -617,7 +617,7 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \tfor_each_ref(handle_one_ref, &list);\n \tfor (l = list; l; l = l->next)\n \t\tcommit_list_insert(l->item, &backup);\n-\twhile ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n+\twhile (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n \t\tchar *p;\n \t\tparse_object(commit->object.sha1);\n \t\tif (!commit->buffer || !(p = strstr(commit->buffer, \"\\n\\n\")))\n--\n1.5.0.3.316.gbd1fc\n"},{"id":"36842","messageId":"Pine.LNX.4.63.0703112008020.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"7198","inReplyTo":"87d53fsjiz.fsf@rho.meyering.net","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-03-11T19:08:43Z","receivedAt":"2007-03-11T19:08:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 11 Mar 2007, Jim Meyering wrote:\n\n> I like the idea of the new ':/' notation, and gave it a \n> try, but all I could get was a segfault.\n\nThanks. I did not realize that it did not work correctly, since the pager \nhid the segfault.\n\nCiao,\nDscho\n"},{"id":"36853","messageId":"Pine.LNX.4.64.0703111322180.9690@woody.linux-foundation.org","threadId":"7198","inReplyTo":"87d53fsjiz.fsf@rho.meyering.net","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-11T20:25:33Z","receivedAt":"2007-03-11T20:25:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 11 Mar 2007, Jim Meyering wrote:\n>\n> I like the idea of the new ':/' notation, and gave it\n> a try, but all I could get was a segfault. It was dereferencing a NULL\n> commit list. Fix below. With it, this example now works:\n\nThe fix is correct, but not complete.\n\n> -\twhile ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n> +\twhile (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n\nThe old code was broken, but the new one isn't much better.\n\n\"pop_most_recent_commit()\" simply doesn't work that way. It *never* \nreturns NULL. So having it as part of a while-loop was buggy to begin \nwith, and you fixed the test, but the thing is, it should just look like\n\n\twhile (list) {\n\t\tstruct commit *commit;\n\n\t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n\t\t..\n\nand the \"pop_most_recent_commit()\" simply shouldn't be part of the \nconditional at all.\n\nAlternatively, we could just change the semantics, and have it return NULL \nwhen the list is empty. That would probably be fine too, but then the old \ncode was correct.\n\nHmm?\n\n\t\tLinus\n"},{"id":"36860","messageId":"7vslcbv6ol.fsf@assigned-by-dhcp.cox.net","threadId":"7198","inReplyTo":"Pine.LNX.4.64.0703111322180.9690@woody.linux-foundation.org","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-11T20:58:18Z","receivedAt":"2007-03-11T20:58:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Sun, 11 Mar 2007, Jim Meyering wrote:\n>>\n>> I like the idea of the new ':/' notation, and gave it\n>> a try, but all I could get was a segfault. It was dereferencing a NULL\n>> commit list. Fix below. With it, this example now works:\n>\n> The fix is correct, but not complete.\n>\n>> -\twhile ((commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n>> +\twhile (list && (commit = pop_most_recent_commit(&list, ONELINE_SEEN))) {\n>\n> The old code was broken, but the new one isn't much better.\n>\n> \"pop_most_recent_commit()\" simply doesn't work that way. It *never* \n> returns NULL. So having it as part of a while-loop was buggy to begin \n> with, and you fixed the test, but the thing is, it should just look like\n>\n> \twhile (list) {\n> \t\tstruct commit *commit;\n>\n> \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n> \t\t..\n>\n> and the \"pop_most_recent_commit()\" simply shouldn't be part of the \n> conditional at all.\n\nThat's what I did in my tentative commit based on Jim's patch\n(except \"commit\" is also used to determine the return value from\nthe function).\n"},{"id":"36914","messageId":"Pine.LNX.4.64.0703120900300.9690@woody.linux-foundation.org","threadId":"7198","inReplyTo":"7vslcbv6ol.fsf@assigned-by-dhcp.cox.net","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-12T16:14:03Z","receivedAt":"2007-03-12T16:14:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 11 Mar 2007, Junio C Hamano wrote:\n> >\n> > \"pop_most_recent_commit()\" simply doesn't work that way. It *never* \n> > returns NULL. So having it as part of a while-loop was buggy to begin \n> > with, and you fixed the test, but the thing is, it should just look like\n> >\n> > \twhile (list) {\n> > \t\tstruct commit *commit;\n> >\n> > \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n> > \t\t..\n> >\n> > and the \"pop_most_recent_commit()\" simply shouldn't be part of the \n> > conditional at all.\n> \n> That's what I did in my tentative commit based on Jim's patch\n> (except \"commit\" is also used to determine the return value from\n> the function).\n\nYour version is *also* buggy.\n\nAnd the bug is exactly that return value.\n\nYou simply cannot do it that way. You do\n\n\t...\n\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n\tif (!commit)\n\t\tbreak;\n\t...\n\nwhich is totally broken and pointless, and more importantly, *exactly* \nbecause you use \"commit\" outside the loop, it's broken.\n\nIt will now return the last commit it ever saw, whether that commit \nmatches the buffer or not. Because \"commit\" will *never* be set to NULL \never again after the initial assignment!\n\nThe fact is, if you don't do it the way I described:\n\n\twhile (list) {\n\t\tstruct commit *commit;\n\n\t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n\t\t...\n\nyou will almost certainly have a bug. Why? Because the return value of \n\"pop_most_recent_commit()\" simply *makes*no*sense* outside the loop. It's \nthat simple. \n\nOutside the loop, you have to have a *different* return value, namely the \none that matches. Which may be NULL. Something that \npop_most_recent_commit() simply never returns!\n\nSo I repeat my suggestion:\n\n - either use the sequence above\n - or change the semantics of \"pop_most_recent_commit()\" to return NULL if \n the list is empty.\n\nConsidering that there's now been two totally broken versions of this \nloop, with the exact same bug, I'm inclined to say that you should just \nchange \"pop_most_recent_commit()\" instead, and then change the loop back \nto its original form of \"while ((commit = pop_..) != NULL) {\"\n\nOtherwise, please apply something like this, which also fixes the return \nvalue. We should return -1 on error!\n\nWe should add a test with\n\n\tgit log :/hjashjs\n\nor similar. It's supposed to return an error (\"fatal: ambiguous argument \n':/hjashjs': unknown revision or path not in the working tree.\")\n\n\t\tLinus\n\n [ Same return value rules as for the kernel and for system calls, either:\n\n - 0/1 for \"false/true\", where _1_ is success, and 0 is \"false\".\n\n - negative/0 for \"error/no-error\", where _0_ is success, and negative \n is a failure.\n\n but not some mixture with 1 being failure! ]\n---\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 6b8b67b..40dd2d1 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -602,10 +602,10 @@ static int handle_one_ref(const char *path,\n */\n \n #define ONELINE_SEEN (1u<<20)\n-int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n+static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n {\n \tstruct commit_list *list = NULL, *backup = NULL, *l;\n-\tstruct commit *commit = NULL;\n+\tint retval = -1;\n \n \tif (prefix[0] == '!') {\n \t\tif (prefix[1] != '!')\n@@ -619,6 +619,7 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \t\tcommit_list_insert(l->item, &backup);\n \twhile (list) {\n \t\tchar *p;\n+\t\tstruct commit *commit;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tif (!commit)\n@@ -628,13 +629,14 @@ int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \t\t\tcontinue;\n \t\tif (!prefixcmp(p + 2, prefix)) {\n \t\t\thashcpy(sha1, commit->object.sha1);\n+\t\t\tretval = 0;\n \t\t\tbreak;\n \t\t}\n \t}\n \tfree_commit_list(list);\n \tfor (l = backup; l; l = l->next)\n \t\tclear_commit_marks(l->item, ONELINE_SEEN);\n-\treturn commit == NULL;\n+\treturn retval;\n }\n \n /*\n"},{"id":"36916","messageId":"Pine.LNX.4.64.0703120921080.9690@woody.linux-foundation.org","threadId":"7198","inReplyTo":"Pine.LNX.4.64.0703120900300.9690@woody.linux-foundation.org","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-12T16:22:09Z","receivedAt":"2007-03-12T16:22:09Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 12 Mar 2007, Linus Torvalds wrote:\n> \n> Otherwise, please apply something like this, which also fixes the return \n> value. We should return -1 on error!\n\nBtw, I didn't remove the \n\n\tif (!commit)\n\t\tbreak;\n\nbut I should have. It doesn't actually matter (since it will never \ntrigger), but it adds to the confusion around pop_most_recent_commit.\n\n\t\tLinus\n"},{"id":"36937","messageId":"7vzm6is3r0.fsf@assigned-by-dhcp.cox.net","threadId":"7198","inReplyTo":"Pine.LNX.4.64.0703120900300.9690@woody.linux-foundation.org","subject":"Re: \"git-diff -p :/anything\" always segfaults","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-12T18:42:11Z","receivedAt":"2007-03-12T18:42:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Your version is *also* buggy.\n\nYou are right.\n\nApplied, and thanks.\n"}]}