{"thread":{"id":"33954","subject":"[PATCH] fix segfault with git log -c --follow","startedAt":"2013-05-27T22:49:57Z","lastAt":"2013-05-28T23:24:55Z","messageCount":4,"participants":["Clemens Buchacher","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"218628","messageId":"20130527224957.GA7492@ecki","threadId":"33954","inReplyTo":null,"subject":"[PATCH] fix segfault with git log -c --follow","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2013-05-27T22:49:57Z","receivedAt":"2013-05-27T22:49:57Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In diff_tree_combined we make a copy of diffopts. In\ntry_to_follow_renames, called via diff_tree_sha1, we free and\nre-initialize diffopts->pathspec->items. Since we did not make a deep\ncopy of diffopts in diff_tree_combined, the original diffopts does not\nget the update. By the time we return from diff_tree_combined,\nrev->diffopt->pathspec->items points to an invalid memory address. We\nget a segfault next time we try to access that pathspec.\n\nInstead, along with the copy of diffopts, make a copy pathspec->items as\nwell.\n\nWe would also have to make a copy of pathspec->raw to keep it consistent\nwith pathspec->items, but nobody seems to rely on that.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\nI wonder why I get a segfault from this so reliably, since it's not\nactually a null-pointer dereference. Maybe this is gcc 4.8 doing\nsomething different from previous versions?\n\nAlso, I have absolutely no confidence in my understanding of this code.\nThis is the first solution that came to mind, and could be totally\nwrong. I just figured a patch is better than no patch.\n\nClemens\n\n combine-diff.c |  3 +++\n t/t4202-log.sh | 14 ++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 77d7872..8825604 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1302,6 +1302,7 @@ void diff_tree_combined(const unsigned char *sha1,\n \tint i, num_paths, needsep, show_log_first, num_parent = parents->nr;\n \n \tdiffopts = *opt;\n+\tdiff_tree_setup_paths(diffopts.pathspec.raw, &diffopts);\n \tdiffopts.output_format = DIFF_FORMAT_NO_OUTPUT;\n \tDIFF_OPT_SET(&diffopts, RECURSIVE);\n \tDIFF_OPT_CLR(&diffopts, ALLOW_EXTERNAL);\n@@ -1372,6 +1373,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\tpaths = paths->next;\n \t\tfree(tmp);\n \t}\n+\n+\tdiff_tree_release_paths(&diffopts);\n }\n \n void diff_tree_combined_merge(const struct commit *commit, int dense,\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 9243a97..cb03d28 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -530,6 +530,20 @@ test_expect_success 'show added path under \"--follow -M\"' '\n \t)\n '\n \n+test_expect_success 'git log -c --follow' '\n+\ttest_create_repo follow-c &&\n+\t(\n+\t\tcd follow-c &&\n+\t\ttest_commit initial file original &&\n+\t\tgit rm file &&\n+\t\ttest_commit rename file2 original &&\n+\t\tgit reset --hard initial &&\n+\t\ttest_commit modify file foo &&\n+\t\tgit merge -m merge rename &&\n+\t\tgit log -c --follow file2\n+\t)\n+'\n+\n cat >expect <<\\EOF\n *   commit COMMIT_OBJECT_NAME\n |\\  Merge: MERGE_PARENTS\n-- \n1.8.2.3\n"},{"id":"218685","messageId":"7vk3mj10l2.fsf@alter.siamese.dyndns.org","threadId":"33954","inReplyTo":"20130527224957.GA7492@ecki","subject":"Re: [PATCH] fix segfault with git log -c --follow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T17:22:17Z","receivedAt":"2013-05-28T17:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n> In diff_tree_combined we make a copy of diffopts. In\n> try_to_follow_renames, called via diff_tree_sha1, we free and\n> re-initialize diffopts->pathspec->items. Since we did not make a deep\n> copy of diffopts in diff_tree_combined, the original diffopts does not\n> get the update. By the time we return from diff_tree_combined,\n> rev->diffopt->pathspec->items points to an invalid memory address. We\n> get a segfault next time we try to access that pathspec.\n\nI am not quite sure if I follow.  Do you mean\n\n\tdiff_tree_combined()\n        - makes a shallow copy of rev->diffopt\n        - calls diff_tree_sha1()\n          diff_tree_sha1()\n          - tries to follow rename and clobbers diffopt\n        - tries to use the shallow copy of original rev->diffopt\n          that no longer is valid, which is a problem\n\nI wonder, just like we force recursive and disable external on the\ncopy before we use it to call diff_tree_sha1(), if we should disable\nfollow-renames on it.  \"--follow\" is an option that is given to the\nhistory traversal part and it should not play any role in getting\nthe pairwise diff with all parents diff_tree_combined() does.\n\nBesides,\n\n - \"--follow\" hack lets us keep track of only one path; and\n\n - \"-c\" and \"--cc\" make sense only when dealing with a merge commit\n   and the path in the child may have come from different path in\n   parents,\n\nso I am not sure if allowing combination of \"--follow -c/--cc\" makes\nmuch sense in the first place.\n"},{"id":"218709","messageId":"20130528225453.GA9820@ecki","threadId":"33954","inReplyTo":"7vk3mj10l2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix segfault with git log -c --follow","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2013-05-28T22:54:54Z","receivedAt":"2013-05-28T22:54:54Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Tue, May 28, 2013 at 10:22:17AM -0700, Junio C Hamano wrote:\n> Clemens Buchacher <drizzd@aon.at> writes:\n> \n> > In diff_tree_combined we make a copy of diffopts. In\n> > try_to_follow_renames, called via diff_tree_sha1, we free and\n> > re-initialize diffopts->pathspec->items. Since we did not make a deep\n> > copy of diffopts in diff_tree_combined, the original diffopts does not\n> > get the update. By the time we return from diff_tree_combined,\n> > rev->diffopt->pathspec->items points to an invalid memory address. We\n> > get a segfault next time we try to access that pathspec.\n> \n> I am not quite sure if I follow.  Do you mean\n> \n> \tdiff_tree_combined()\n>         - makes a shallow copy of rev->diffopt\n>         - calls diff_tree_sha1()\n>           diff_tree_sha1()\n>           - tries to follow rename and clobbers diffopt\n\nRight.\n\n>         - tries to use the shallow copy of original rev->diffopt\n>           that no longer is valid, which is a problem\n\ndiff_tree_combined does not try to use it right away. It does return,\nbut rev->diffopt is now invalid and the next time we do any kind of diff\nwith it, we have a problem.\n\n> I wonder, just like we force recursive and disable external on the\n> copy before we use it to call diff_tree_sha1(), if we should disable\n> follow-renames on it.  \"--follow\" is an option that is given to the\n> history traversal part and it should not play any role in getting the\n> pairwise diff with all parents diff_tree_combined() does.\n\nCan't parse that last sentence.\n\nIn any case, I don't think disabling diff_tree_sha1 is a solution. The\nbug is in diff_tree_sha1 and its subfunctions, because they manipulate a\ndata structures such that it becomes corrupt. And they do so in an\nobfuscated and clearly unintentional manner. So we should not blame the\nuser for calling diff_tree_sha1 in such a way that it causes corruption.\n\n> Besides,\n> \n>  - \"--follow\" hack lets us keep track of only one path; and\n\nOk. Good to know it is considered a hack. The code is quite strange\nindeed.\n\n>  - \"-c\" and \"--cc\" make sense only when dealing with a merge commit\n>    and the path in the child may have come from different path in\n>    parents,\n\nSorry, I don't get it.\n\n> so I am not sure if allowing combination of \"--follow -c/--cc\" makes\n> much sense in the first place.\n\nMy use-case is came up with this history:\n\n1. Code gets added to file A.\n2. File A gets renamed to B in a different branch.\n3. The branches get merged, and code from (1) is removed in the merge.\n\nLater I wonder why code from (1) is gone from B even though I felt\ncertain it had been added before. I also remember that B was renamed at\nsome point. So I do git log -p --follow B, and it nicely shows that diff\nwhere the code was added, but no diff where the code is removed.\n\nThe reason is of course, that the code was removed in the merge and that\ndiff is not shown. And -c is usually what I do to enable showing diffs\nin merge commits.\n\nAnd if the pairwise diff can also deal with file renames, I think it\nabsolutely does make sense to show also a three-way diff.\n\nI can't tell far away the code is from supporting anything like that.\n\nCheers,\nClemens\n"},{"id":"218710","messageId":"7vr4gqwuuw.fsf@alter.siamese.dyndns.org","threadId":"33954","inReplyTo":"20130528225453.GA9820@ecki","subject":"Re: [PATCH] fix segfault with git log -c --follow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T23:24:55Z","receivedAt":"2013-05-28T23:24:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher <drizzd@aon.at> writes:\n\n>> I wonder, just like we force recursive and disable external on the\n>> copy before we use it to call diff_tree_sha1(), if we should disable\n>> follow-renames on it.  \"--follow\" is an option that is given to the\n>> history traversal part and it should not play any role in getting the\n>> pairwise diff with all parents diff_tree_combined() does.\n>\n> Can't parse that last sentence.\n>\n> In any case, I don't think disabling diff_tree_sha1 is a solution. The\n> bug is in diff_tree_sha1 and its subfunctions, because they manipulate a\n> data structures such that it becomes corrupt. And they do so in an\n> obfuscated and clearly unintentional manner. So we should not blame the\n> user for calling diff_tree_sha1 in such a way that it causes corruption.\n>\n>> Besides,\n>> \n>>  - \"--follow\" hack lets us keep track of only one path; and\n>\n> Ok. Good to know it is considered a hack. The code is quite strange\n> indeed.\n\nThe problem with --follow is that it only tracks one path globally.\nIn a history like this, suppose that a path X long time ago was\nrenamed to Y at commit B:\n\n    ---o---A---B---C---o HEAD\n\nand you start digging with \"log --follow -c HEAD -- Y\".  When\nlooking at C, because it and its parent B both have path Y, the\ntry-to-follow hack does not kick in, and when trying to show C, we\nwill show the change in Y (because that is the pathspec).\n\nThen we look at B.  Because B has path we are following, i.e. Y, and\nits parent A does not, try-to-follow hack kicks in, and it mangles\nthe pathspec that is used globally for history traversal to X while\nshowing the difference between A's X and B's Y.  Then we dig further\nto find A; at this point the global pathspec is swapped and now it\nis X.\n\nThat makes --follow a working hack for a simplest single strand of\npearls.  But if you have a mergy history, e.g.\n\n    ---o---A---------------B---C---o HEAD\n            \\                 /\n             D---E---F---G---H\n\nit can break in interesting ways.  We are likely to have looked at H\nbefore looking at B and used pathspec Y while inspecting H, but\nafter looking at B, the global pathspec is swapped to X, and then we\ntry to look at G, F, E and D, none of which may have renamed the\noriginal X, so you would likely miss the change to the path Y you\nwanted to follow.\n\nTo fix this, we would need to keep \"what path are we following\" not\nin the global revs->pathspec, but per the traversal paths that are\ncurrently active (e.g. when we look at C and H, it is Y, when we\nlook at B, it is X, when we look at G, that is inherited from H and\nstill Y, not affected by the rename at B.  And then when we look at\nA (we need topo-order traversal to do this), it needs to notice that\none child (i.e. B) has been following X while the other (i.e. D) Y,\nand merge the \"I've been following this path\" information in a\nsensible way (e.g. look at its own tree and see what is available,\nin this case X).\n"}]}