{"thread":{"id":"30331","subject":"Possible segfault introduced in commit.c","startedAt":"2012-04-25T07:59:28Z","lastAt":"2012-04-30T21:07:21Z","messageCount":8,"participants":["Michael Mueller","Jeff King","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"190045","messageId":"CANV9Rr_ev+34Wd030cps0UbgjRYD0=L2DQhbrCOkBVWG-2xaug@mail.gmail.com","threadId":"30331","inReplyTo":null,"subject":"Possible segfault introduced in commit.c","fromName":"Michael Mueller","fromEmail":"mmueller@vigilantsw.com","sentAt":"2012-04-25T07:59:28Z","receivedAt":"2012-04-25T07:59:28Z","isPatch":false,"sender":{"key":"mmueller@vigilantsw.com","avatar":null},"body":"Hi all,\n\nAs you might already know, we analyze git regularly with Sentry (our\nstatic analysis tool).  Today it picked up a new NULL pointer\ndereference in commit.c:366:\n\n    void commit_list_reverse(struct commit_list **list_p)\n    {\n        struct commit_list *prev = NULL, *curr = *list_p, *next;\n\n        if (!list_p)\n            return;\n        /* function continues... */\n    }\n\nlist_p is dereferenced on the first line, then tested for NULL on\nthe very next statement.  If it's possible that list_p is NULL, this\nwill be a segfault.  If it can't be NULL, then the check is\nunnecessary (and probably misleading).\n\nIntroduced here:\nhttps://github.com/gitster/git/commit/fbc08ea\n\nBest,\nMike\n\n-- \nMike Mueller\nPhone: (401) 405-1525\nEmail: mmueller@vigilantsw.com\n\nhttp://www.vigilantsw.com/\n"},{"id":"190052","messageId":"20120425111435.GA21579@sigill.intra.peff.net","threadId":"30331","inReplyTo":"CANV9Rr_ev+34Wd030cps0UbgjRYD0=L2DQhbrCOkBVWG-2xaug@mail.gmail.com","subject":"Re: Possible segfault introduced in commit.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-25T11:14:35Z","receivedAt":"2012-04-25T11:14:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 25, 2012 at 12:59:28AM -0700, Michael Mueller wrote:\n\n> As you might already know, we analyze git regularly with Sentry (our\n> static analysis tool).  Today it picked up a new NULL pointer\n> dereference in commit.c:366:\n> \n>     void commit_list_reverse(struct commit_list **list_p)\n>     {\n>         struct commit_list *prev = NULL, *curr = *list_p, *next;\n> \n>         if (!list_p)\n>             return;\n>         /* function continues... */\n>     }\n> \n> list_p is dereferenced on the first line, then tested for NULL on\n> the very next statement.  If it's possible that list_p is NULL, this\n> will be a segfault.  If it can't be NULL, then the check is\n> unnecessary (and probably misleading).\n\nYes, you're right. There is only one caller currently, and it can never\nbe NULL (it passes the address-of a pointer variable). I think dropping\nthe NULL-check is the right thing; even an empty list will still have a\npointer to its NULL head.\n\n-Peff\n"},{"id":"190077","messageId":"4F985D0A.9020100@lsrfire.ath.cx","threadId":"30331","inReplyTo":"20120425111435.GA21579@sigill.intra.peff.net","subject":"Re: Possible segfault introduced in commit.c","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-04-25T20:22:34Z","receivedAt":"2012-04-25T20:22:34Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.04.2012 13:14, schrieb Jeff King:\n> On Wed, Apr 25, 2012 at 12:59:28AM -0700, Michael Mueller wrote:\n>\n>> As you might already know, we analyze git regularly with Sentry (our\n>> static analysis tool).  Today it picked up a new NULL pointer\n>> dereference in commit.c:366:\n>>\n>>      void commit_list_reverse(struct commit_list **list_p)\n>>      {\n>>          struct commit_list *prev = NULL, *curr = *list_p, *next;\n>>\n>>          if (!list_p)\n>>              return;\n>>          /* function continues... */\n>>      }\n>>\n>> list_p is dereferenced on the first line, then tested for NULL on\n>> the very next statement.  If it's possible that list_p is NULL, this\n>> will be a segfault.  If it can't be NULL, then the check is\n>> unnecessary (and probably misleading).\n>\n> Yes, you're right. There is only one caller currently, and it can never\n> be NULL (it passes the address-of a pointer variable). I think dropping\n> the NULL-check is the right thing; even an empty list will still have a\n> pointer to its NULL head.\n\nMore often then not, a mistake like that is surrounded by other issues. \n  No, I didn't put it there intentionally to prove this point. ;-)\n\nHaving to reverse the list at all is unfortunate and I only did that \nbecause I thought appending would be more complicated and because we are \ngoing to replace the linked list with a different data structure soon \nanyway.  Turns out appending is easy.  Patches to follow.\n\nRené\n"},{"id":"190078","messageId":"4F98600F.6000404@lsrfire.ath.cx","threadId":"30331","inReplyTo":"4F985D0A.9020100@lsrfire.ath.cx","subject":"[PATCH 1/3] sequencer: export commit_list_append()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-04-25T20:35:27Z","receivedAt":"2012-04-25T20:35:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"This function can be used in other parts of git.  Give it a new home\nin commit.c.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n commit.c    |   27 +++++++++++++++++++++++++++\n commit.h    |    2 ++\n sequencer.c |   27 ---------------------------\n 3 files changed, 29 insertions(+), 27 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex b80a452..8361acb 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1214,3 +1214,30 @@ struct commit *get_merge_parent(const char *name)\n \t}\n \treturn commit;\n }\n+\n+/*\n+ * Append a commit to the end of the commit_list.\n+ *\n+ * next starts by pointing to the variable that holds the head of an\n+ * empty commit_list, and is updated to point to the \"next\" field of\n+ * the last item on the list as new commits are appended.\n+ *\n+ * Usage example:\n+ *\n+ *     struct commit_list *list;\n+ *     struct commit_list **next = &list;\n+ *\n+ *     next = commit_list_append(c1, next);\n+ *     next = commit_list_append(c2, next);\n+ *     assert(commit_list_count(list) == 2);\n+ *     return list;\n+ */\n+struct commit_list **commit_list_append(struct commit *commit,\n+\t\t\t\t\tstruct commit_list **next)\n+{\n+\tstruct commit_list *new = xmalloc(sizeof(struct commit_list));\n+\tnew->item = commit;\n+\t*next = new;\n+\tnew->next = NULL;\n+\treturn &new->next;\n+}\ndiff --git a/commit.h b/commit.h\nindex f8d250d..bd17770 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -53,6 +53,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject);\n \n struct commit_list *commit_list_insert(struct commit *item,\n \t\t\t\t\tstruct commit_list **list);\n+struct commit_list **commit_list_append(struct commit *commit,\n+\t\t\t\t\tstruct commit_list **next);\n unsigned commit_list_count(const struct commit_list *l);\n struct commit_list *commit_list_insert_by_date(struct commit *item,\n \t\t\t\t    struct commit_list **list);\ndiff --git a/sequencer.c b/sequencer.c\nindex 4307364..ac6c823 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -468,33 +468,6 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n \trollback_lock_file(&index_lock);\n }\n \n-/*\n- * Append a commit to the end of the commit_list.\n- *\n- * next starts by pointing to the variable that holds the head of an\n- * empty commit_list, and is updated to point to the \"next\" field of\n- * the last item on the list as new commits are appended.\n- *\n- * Usage example:\n- *\n- *     struct commit_list *list;\n- *     struct commit_list **next = &list;\n- *\n- *     next = commit_list_append(c1, next);\n- *     next = commit_list_append(c2, next);\n- *     assert(commit_list_count(list) == 2);\n- *     return list;\n- */\n-static struct commit_list **commit_list_append(struct commit *commit,\n-\t\t\t\t\t       struct commit_list **next)\n-{\n-\tstruct commit_list *new = xmalloc(sizeof(struct commit_list));\n-\tnew->item = commit;\n-\t*next = new;\n-\tnew->next = NULL;\n-\treturn &new->next;\n-}\n-\n static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \t\tstruct replay_opts *opts)\n {\n-- \n1.7.10\n"},{"id":"190079","messageId":"4F98601D.3070303@lsrfire.ath.cx","threadId":"30331","inReplyTo":"4F985D0A.9020100@lsrfire.ath.cx","subject":"[PATCH 2/3] revision: append to list instead of insert and reverse","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-04-25T20:35:41Z","receivedAt":"2012-04-25T20:35:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"By using commit_list_insert(), we added new items to the top of the\nlist and, since this is not the order we want, reversed it afterwards.\nSimplify this process by adding new items at the bottom instead,\ngetting rid of the reversal step.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n revision.c |    4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex edb225d..9e63baa 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2071,6 +2071,7 @@ int prepare_revision_walk(struct rev_info *revs)\n {\n \tint nr = revs->pending.nr;\n \tstruct object_array_entry *e, *list;\n+\tstruct commit_list **next = &revs->commits;\n \n \te = list = revs->pending.objects;\n \trevs->pending.nr = 0;\n@@ -2081,12 +2082,11 @@ int prepare_revision_walk(struct rev_info *revs)\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n \t\t\t\tcommit->object.flags |= SEEN;\n-\t\t\t\tcommit_list_insert(commit, &revs->commits);\n+\t\t\t\tnext = commit_list_append(commit, next);\n \t\t\t}\n \t\t}\n \t\te++;\n \t}\n-\tcommit_list_reverse(&revs->commits);\n \tcommit_list_sort_by_date(&revs->commits);\n \tif (!revs->leak_pending)\n \t\tfree(list);\n-- \n1.7.10\n"},{"id":"190080","messageId":"4F98602A.7020404@lsrfire.ath.cx","threadId":"30331","inReplyTo":"4F985D0A.9020100@lsrfire.ath.cx","subject":"[PATCH 3/3] commit: remove commit_list_reverse()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-04-25T20:35:54Z","receivedAt":"2012-04-25T20:35:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The function commit_list_reverse() is not used anymore; delete it.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n commit.c |   15 ---------------\n commit.h |    1 -\n 2 files changed, 16 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 8361acb..9ed36c7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -361,21 +361,6 @@ struct commit_list *commit_list_insert(struct commit *item, struct commit_list *\n \treturn new_list;\n }\n \n-void commit_list_reverse(struct commit_list **list_p)\n-{\n-\tstruct commit_list *prev = NULL, *curr = *list_p, *next;\n-\n-\tif (!list_p)\n-\t\treturn;\n-\twhile (curr) {\n-\t\tnext = curr->next;\n-\t\tcurr->next = prev;\n-\t\tprev = curr;\n-\t\tcurr = next;\n-\t}\n-\t*list_p = prev;\n-}\n-\n unsigned commit_list_count(const struct commit_list *l)\n {\n \tunsigned c = 0;\ndiff --git a/commit.h b/commit.h\nindex bd17770..ccaa20b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -59,7 +59,6 @@ unsigned commit_list_count(const struct commit_list *l);\n struct commit_list *commit_list_insert_by_date(struct commit *item,\n \t\t\t\t    struct commit_list **list);\n void commit_list_sort_by_date(struct commit_list **list);\n-void commit_list_reverse(struct commit_list **list_p);\n \n void free_commit_list(struct commit_list *list);\n \n-- \n1.7.10\n"},{"id":"190084","messageId":"xmqqobqfbhae.fsf@junio.mtv.corp.google.com","threadId":"30331","inReplyTo":"4F98600F.6000404@lsrfire.ath.cx","subject":"Re: [PATCH 1/3] sequencer: export commit_list_append()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-25T22:03:37Z","receivedAt":"2012-04-25T22:03:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> This function can be used in other parts of git.  Give it a new home\n> in commit.c.\n>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n\nThis makes sense.  I got confused every time I had to \"append to tail\"\nand had to draw boxes-and-arrows picture to make sure I understand how\nto use \"\\(.*\\) = &commit_list_insert(something, \\1)->next\" correctly.\n\nThere probably are tons of places that can use this thing.\n\n    $ git grep -c -e '\\&commit_list_insert(.*)->next'\n    builtin/commit.c:4\n    builtin/diff-tree.c:1\n    builtin/merge.c:3\n    commit.c:4\n    revision.c:5\n\nI however wonder if we can name \"next\" a bit better, but cannot come up\nwith a good name.  It is the location that holds the pointer to the new\ntail element if we append one.  Some places may call it \"tail\" but that\ngives a wrong impression that it points at the element at the end.\n"},{"id":"190344","messageId":"4F9EFF09.5010201@lsrfire.ath.cx","threadId":"30331","inReplyTo":"xmqqobqfbhae.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 1/3] sequencer: export commit_list_append()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-04-30T21:07:21Z","receivedAt":"2012-04-30T21:07:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.04.2012 00:03, schrieb Junio C Hamano:\n> I however wonder if we can name \"next\" a bit better, but cannot come up\n> with a good name.  It is the location that holds the pointer to the new\n> tail element if we append one.  Some places may call it \"tail\" but that\n> gives a wrong impression that it points at the element at the end.\n\nPerhaps tail_next?  And it's perhaps a good idea to include it in the \nstruct instead of letting callers maintain it separately.\n\nRené\n"}]}