# Possible segfault introduced in commit.c 8 messages from 2012-04-25 to 2012-04-30. Participants: Michael Mueller, Jeff King, René Scharfe, Junio C Hamano. Thread: https://gitlist.dev/t/30331 ## Michael Mueller, 2012-04-25 07:59 Subject: Possible segfault introduced in commit.c Message-ID: URL: https://gitlist.dev/e/CANV9Rr_ev%2B34Wd030cps0UbgjRYD0%3DL2DQhbrCOkBVWG-2xaug%40mail.gmail.com ``` Hi all, As you might already know, we analyze git regularly with Sentry (our static analysis tool). Today it picked up a new NULL pointer dereference in commit.c:366: void commit_list_reverse(struct commit_list **list_p) { struct commit_list *prev = NULL, *curr = *list_p, *next; if (!list_p) return; /* function continues... */ } list_p is dereferenced on the first line, then tested for NULL on the very next statement. If it's possible that list_p is NULL, this will be a segfault. If it can't be NULL, then the check is unnecessary (and probably misleading). Introduced here: https://github.com/gitster/git/commit/fbc08ea Best, Mike -- Mike Mueller Phone: (401) 405-1525 Email: mmueller@vigilantsw.com http://www.vigilantsw.com/ ``` ## Jeff King, 2012-04-25 11:14 Subject: Re: Possible segfault introduced in commit.c Message-ID: <20120425111435.GA21579@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120425111435.GA21579%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Apr 25, 2012 at 12:59:28AM -0700, Michael Mueller wrote: > As you might already know, we analyze git regularly with Sentry (our > static analysis tool). Today it picked up a new NULL pointer > dereference in commit.c:366: > > void commit_list_reverse(struct commit_list **list_p) > { > struct commit_list *prev = NULL, *curr = *list_p, *next; > > if (!list_p) > return; > /* function continues... */ > } > > list_p is dereferenced on the first line, then tested for NULL on > the very next statement. If it's possible that list_p is NULL, this > will be a segfault. If it can't be NULL, then the check is > unnecessary (and probably misleading). Yes, you're right. There is only one caller currently, and it can never be NULL (it passes the address-of a pointer variable). I think dropping the NULL-check is the right thing; even an empty list will still have a pointer to its NULL head. -Peff ``` ## René Scharfe, 2012-04-25 20:22 Subject: Re: Possible segfault introduced in commit.c Message-ID: <4F985D0A.9020100@lsrfire.ath.cx> URL: https://gitlist.dev/e/4F985D0A.9020100%40lsrfire.ath.cx In-Reply-To: <20120425111435.GA21579@sigill.intra.peff.net> ``` Am 25.04.2012 13:14, schrieb Jeff King: > On Wed, Apr 25, 2012 at 12:59:28AM -0700, Michael Mueller wrote: > >> As you might already know, we analyze git regularly with Sentry (our >> static analysis tool). Today it picked up a new NULL pointer >> dereference in commit.c:366: >> >> void commit_list_reverse(struct commit_list **list_p) >> { >> struct commit_list *prev = NULL, *curr = *list_p, *next; >> >> if (!list_p) >> return; >> /* function continues... */ >> } >> >> list_p is dereferenced on the first line, then tested for NULL on >> the very next statement. If it's possible that list_p is NULL, this >> will be a segfault. If it can't be NULL, then the check is >> unnecessary (and probably misleading). > > Yes, you're right. There is only one caller currently, and it can never > be NULL (it passes the address-of a pointer variable). I think dropping > the NULL-check is the right thing; even an empty list will still have a > pointer to its NULL head. More often then not, a mistake like that is surrounded by other issues. No, I didn't put it there intentionally to prove this point. ;-) Having to reverse the list at all is unfortunate and I only did that because I thought appending would be more complicated and because we are going to replace the linked list with a different data structure soon anyway. Turns out appending is easy. Patches to follow. René ``` ## René Scharfe, 2012-04-25 20:35 Subject: [PATCH 1/3] sequencer: export commit_list_append() Message-ID: <4F98600F.6000404@lsrfire.ath.cx> URL: https://gitlist.dev/e/4F98600F.6000404%40lsrfire.ath.cx In-Reply-To: <4F985D0A.9020100@lsrfire.ath.cx> ``` This function can be used in other parts of git. Give it a new home in commit.c. Signed-off-by: Rene Scharfe --- commit.c | 27 +++++++++++++++++++++++++++ commit.h | 2 ++ sequencer.c | 27 --------------------------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/commit.c b/commit.c index b80a452..8361acb 100644 --- a/commit.c +++ b/commit.c @@ -1214,3 +1214,30 @@ struct commit *get_merge_parent(const char *name) } return commit; } + +/* + * Append a commit to the end of the commit_list. + * + * next starts by pointing to the variable that holds the head of an + * empty commit_list, and is updated to point to the "next" field of + * the last item on the list as new commits are appended. + * + * Usage example: + * + * struct commit_list *list; + * struct commit_list **next = &list; + * + * next = commit_list_append(c1, next); + * next = commit_list_append(c2, next); + * assert(commit_list_count(list) == 2); + * return list; + */ +struct commit_list **commit_list_append(struct commit *commit, + struct commit_list **next) +{ + struct commit_list *new = xmalloc(sizeof(struct commit_list)); + new->item = commit; + *next = new; + new->next = NULL; + return &new->next; +} diff --git a/commit.h b/commit.h index f8d250d..bd17770 100644 --- a/commit.h +++ b/commit.h @@ -53,6 +53,8 @@ int find_commit_subject(const char *commit_buffer, const char **subject); struct commit_list *commit_list_insert(struct commit *item, struct commit_list **list); +struct commit_list **commit_list_append(struct commit *commit, + struct commit_list **next); unsigned commit_list_count(const struct commit_list *l); struct commit_list *commit_list_insert_by_date(struct commit *item, struct commit_list **list); diff --git a/sequencer.c b/sequencer.c index 4307364..ac6c823 100644 --- a/sequencer.c +++ b/sequencer.c @@ -468,33 +468,6 @@ static void read_and_refresh_cache(struct replay_opts *opts) rollback_lock_file(&index_lock); } -/* - * Append a commit to the end of the commit_list. - * - * next starts by pointing to the variable that holds the head of an - * empty commit_list, and is updated to point to the "next" field of - * the last item on the list as new commits are appended. - * - * Usage example: - * - * struct commit_list *list; - * struct commit_list **next = &list; - * - * next = commit_list_append(c1, next); - * next = commit_list_append(c2, next); - * assert(commit_list_count(list) == 2); - * return list; - */ -static struct commit_list **commit_list_append(struct commit *commit, - struct commit_list **next) -{ - struct commit_list *new = xmalloc(sizeof(struct commit_list)); - new->item = commit; - *next = new; - new->next = NULL; - return &new->next; -} - static int format_todo(struct strbuf *buf, struct commit_list *todo_list, struct replay_opts *opts) { -- 1.7.10 ``` ## René Scharfe, 2012-04-25 20:35 Subject: [PATCH 2/3] revision: append to list instead of insert and reverse Message-ID: <4F98601D.3070303@lsrfire.ath.cx> URL: https://gitlist.dev/e/4F98601D.3070303%40lsrfire.ath.cx In-Reply-To: <4F985D0A.9020100@lsrfire.ath.cx> ``` By using commit_list_insert(), we added new items to the top of the list and, since this is not the order we want, reversed it afterwards. Simplify this process by adding new items at the bottom instead, getting rid of the reversal step. Signed-off-by: Rene Scharfe --- revision.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/revision.c b/revision.c index edb225d..9e63baa 100644 --- a/revision.c +++ b/revision.c @@ -2071,6 +2071,7 @@ int prepare_revision_walk(struct rev_info *revs) { int nr = revs->pending.nr; struct object_array_entry *e, *list; + struct commit_list **next = &revs->commits; e = list = revs->pending.objects; revs->pending.nr = 0; @@ -2081,12 +2082,11 @@ int prepare_revision_walk(struct rev_info *revs) if (commit) { if (!(commit->object.flags & SEEN)) { commit->object.flags |= SEEN; - commit_list_insert(commit, &revs->commits); + next = commit_list_append(commit, next); } } e++; } - commit_list_reverse(&revs->commits); commit_list_sort_by_date(&revs->commits); if (!revs->leak_pending) free(list); -- 1.7.10 ``` ## René Scharfe, 2012-04-25 20:35 Subject: [PATCH 3/3] commit: remove commit_list_reverse() Message-ID: <4F98602A.7020404@lsrfire.ath.cx> URL: https://gitlist.dev/e/4F98602A.7020404%40lsrfire.ath.cx In-Reply-To: <4F985D0A.9020100@lsrfire.ath.cx> ``` The function commit_list_reverse() is not used anymore; delete it. Signed-off-by: Rene Scharfe --- commit.c | 15 --------------- commit.h | 1 - 2 files changed, 16 deletions(-) diff --git a/commit.c b/commit.c index 8361acb..9ed36c7 100644 --- a/commit.c +++ b/commit.c @@ -361,21 +361,6 @@ struct commit_list *commit_list_insert(struct commit *item, struct commit_list * return new_list; } -void commit_list_reverse(struct commit_list **list_p) -{ - struct commit_list *prev = NULL, *curr = *list_p, *next; - - if (!list_p) - return; - while (curr) { - next = curr->next; - curr->next = prev; - prev = curr; - curr = next; - } - *list_p = prev; -} - unsigned commit_list_count(const struct commit_list *l) { unsigned c = 0; diff --git a/commit.h b/commit.h index bd17770..ccaa20b 100644 --- a/commit.h +++ b/commit.h @@ -59,7 +59,6 @@ unsigned commit_list_count(const struct commit_list *l); struct commit_list *commit_list_insert_by_date(struct commit *item, struct commit_list **list); void commit_list_sort_by_date(struct commit_list **list); -void commit_list_reverse(struct commit_list **list_p); void free_commit_list(struct commit_list *list); -- 1.7.10 ``` ## Junio C Hamano, 2012-04-25 22:03 Subject: Re: [PATCH 1/3] sequencer: export commit_list_append() Message-ID: URL: https://gitlist.dev/e/xmqqobqfbhae.fsf%40junio.mtv.corp.google.com In-Reply-To: <4F98600F.6000404@lsrfire.ath.cx> ``` René Scharfe writes: > This function can be used in other parts of git. Give it a new home > in commit.c. > > Signed-off-by: Rene Scharfe This makes sense. I got confused every time I had to "append to tail" and had to draw boxes-and-arrows picture to make sure I understand how to use "\(.*\) = &commit_list_insert(something, \1)->next" correctly. There probably are tons of places that can use this thing. $ git grep -c -e '\&commit_list_insert(.*)->next' builtin/commit.c:4 builtin/diff-tree.c:1 builtin/merge.c:3 commit.c:4 revision.c:5 I however wonder if we can name "next" a bit better, but cannot come up with a good name. It is the location that holds the pointer to the new tail element if we append one. Some places may call it "tail" but that gives a wrong impression that it points at the element at the end. ``` ## René Scharfe, 2012-04-30 21:07 Subject: Re: [PATCH 1/3] sequencer: export commit_list_append() Message-ID: <4F9EFF09.5010201@lsrfire.ath.cx> URL: https://gitlist.dev/e/4F9EFF09.5010201%40lsrfire.ath.cx In-Reply-To: ``` Am 26.04.2012 00:03, schrieb Junio C Hamano: > I however wonder if we can name "next" a bit better, but cannot come up > with a good name. It is the location that holds the pointer to the new > tail element if we append one. Some places may call it "tail" but that > gives a wrong impression that it points at the element at the end. Perhaps tail_next? And it's perhaps a good idea to include it in the struct instead of letting callers maintain it separately. René ```