Possible segfault introduced in commit.c

Possible segfault introduced in commit.c

[Michael Mueller]

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/

Re: Possible segfault introduced in commit.c

[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.

-Peff

Re: Possible segfault introduced in commit.c

[René Scharfe]

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é

[PATCH 1/3] sequencer: export commit_list_append()

[René Scharfe]

This function can be used in other parts of git.  Give it a new home
in commit.c.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 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

[PATCH 2/3] revision: append to list instead of insert and reverse

[René Scharfe]

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 <rene.scharfe@lsrfire.ath.cx>
---
 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

[PATCH 3/3] commit: remove commit_list_reverse()

[René Scharfe]

The function commit_list_reverse() is not used anymore; delete it.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 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

Re: [PATCH 1/3] sequencer: export commit_list_append()

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> This function can be used in other parts of git.  Give it a new home
> in commit.c.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>

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.

Re: [PATCH 1/3] sequencer: export commit_list_append()

[René Scharfe]

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é