{"thread":{"id":"39680","subject":"[PATCH] revision.c: Correctly dereference interesting_cache","startedAt":"2015-06-19T19:01:23Z","lastAt":"2015-06-19T21:00:24Z","messageCount":3,"participants":["Stefan Beller","Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"264356","messageId":"1434740483-31730-1-git-send-email-sbeller@google.com","threadId":"39680","inReplyTo":null,"subject":"[PATCH] revision.c: Correctly dereference interesting_cache","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-19T19:01:23Z","receivedAt":"2015-06-19T19:01:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid\nquadratic behavior from still_interesting), which\nalso introduced the check a few lines before, which already dereferences\n`interesting_cache`. So at this point `interesting_cache` is guaranteed to\nbe not NULL. The code is called referencing the address of a local\nvariable, so `interesting_cache` can actually never be NULL and trigger a\nsegmentation fault by dereferencing it a few lines before this.\n\nI think the right thing is to check for `*interesting_cache` as that\ncan become NULL actually.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nHi Jeff,\n\nI found this possible defect via coverity id 1295352.\nAs I have had limited exposure to revision.c code until now,\nthe commit message may or may not be bogus.\n\nThanks,\nStefan\n\n\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 3ff8723..d1f0f07 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -347,35 +347,35 @@ static struct commit *handle_commit(struct rev_info *revs,\n \n static int everybody_uninteresting(struct commit_list *orig,\n \t\t\t\t   struct commit **interesting_cache)\n {\n \tstruct commit_list *list = orig;\n \n \tif (*interesting_cache) {\n \t\tstruct commit *commit = *interesting_cache;\n \t\tif (!(commit->object.flags & UNINTERESTING))\n \t\t\treturn 0;\n \t}\n \n \twhile (list) {\n \t\tstruct commit *commit = list->item;\n \t\tlist = list->next;\n \t\tif (commit->object.flags & UNINTERESTING)\n \t\t\tcontinue;\n-\t\tif (interesting_cache)\n+\t\tif (*interesting_cache)\n \t\t\t*interesting_cache = commit;\n \t\treturn 0;\n \t}\n \treturn 1;\n }\n \n /*\n  * A definition of \"relevant\" commit that we can use to simplify limited graphs\n  * by eliminating side branches.\n  *\n  * A \"relevant\" commit is one that is !UNINTERESTING (ie we are including it\n  * in our list), or that is a specified BOTTOM commit. Then after computing\n  * a limited list, during processing we can generally ignore boundary merges\n  * coming from outside the graph, (ie from irrelevant parents), and treat\n  * those merges as if they were single-parent. TREESAME is defined to consider\n  * only relevant parents, if any. If we are TREESAME to our on-graph parents,\n  * we don't care if we were !TREESAME to non-graph parents.\n-- \n2.4.1.345.gab207b6.dirty\n"},{"id":"264378","messageId":"20150619204923.GA2964@peff.net","threadId":"39680","inReplyTo":"1434740483-31730-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] revision.c: Correctly dereference interesting_cache","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-19T20:49:24Z","receivedAt":"2015-06-19T20:49:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 19, 2015 at 12:01:23PM -0700, Stefan Beller wrote:\n\n> This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid\n> quadratic behavior from still_interesting), which\n> also introduced the check a few lines before, which already dereferences\n> `interesting_cache`. So at this point `interesting_cache` is guaranteed to\n> be not NULL. The code is called referencing the address of a local\n> variable, so `interesting_cache` can actually never be NULL and trigger a\n> segmentation fault by dereferencing it a few lines before this.\n\nYeah, I agree it can never be NULL here or we would have already\nsegfaulted. Thanks for digging into it.\n\n> I think the right thing is to check for `*interesting_cache` as that\n> can become NULL actually.\n\nI don't think this is right. We have found the interesting commit, so we\nwant to write it into the cache unconditionally, not only if there is\nnothing else in the cache (we know if we got here that either there was\nno current cache item, or it already became UNINTERESTING).\n\nI think it is simply a misguided defensive measure to make sure that the\ncaller passed in a cache slot to us. But there is only one caller, and\nthey always pass a cache, so the first part of the function was lazy and\nnot defensive.\n\n>  \twhile (list) {\n>  \t\tstruct commit *commit = list->item;\n>  \t\tlist = list->next;\n>  \t\tif (commit->object.flags & UNINTERESTING)\n>  \t\t\tcontinue;\n> -\t\tif (interesting_cache)\n> +\t\tif (*interesting_cache)\n>  \t\t\t*interesting_cache = commit;\n\nSo I think the right solution is just to drop the conditional entirely.\nThe current code is not wrong (it is always a noop). What you have here\nactually misbehaves; it does not update the cache slot when it has\nbecome UNINTERESTING. That does not produce wrong results, but it loses\nthe benefit of the cache in some cases.\n\n-Peff\n"},{"id":"264379","messageId":"20150619210023.GA4865@google.com","threadId":"39680","inReplyTo":"1434740483-31730-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] revision.c: Correctly dereference interesting_cache","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-06-19T21:00:24Z","receivedAt":"2015-06-19T21:00:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This was introduced at b6e8a3b5 (2015-04-17, limit_list: avoid\n> quadratic behavior from still_interesting), which\n> also introduced the check a few lines before, which already dereferences\n> `interesting_cache`. So at this point `interesting_cache` is guaranteed to\n> be not NULL.\n\nThe above is the rationale for the coverity warning, but it does not\nexplain why this change is safe.\n\n>                The code is called referencing the address of a local\n> variable, so `interesting_cache` can actually never be NULL and trigger a\n> segmentation fault by dereferencing it a few lines before this.\n\nI'm having trouble parsing this sentence.  Do you mean that limit_list()\nonly calls still_interesting() (and thus, indirectly,\neverybody_uninteresting()), with the second parameter equal to the\naddress of the local interesting_cache variable, so it can never be\nNULL?\n\nThat makes sense, but I had to look at the code and reread the above\nsentence a few times before I understood.\n\nDo you know what this code is trying to check for?  What does it mean\nfor *interesting_cache to be NULL?\n\nShould there be\n\n\tif (!interesting_cache)\n\t\tdie(\"BUG: &interesting_cache == NULL\");\n\nchecks at the top of still_interesting and everybody_uninteresting to\nfutureproof this?\n\nWhat does the *interesting_cache variable represent, anyway?\n\nThis code seems to be underdocumented.\n\nThanks and hope that helps,\nJonathan\n"}]}