Possible segfault introduced in commit.c

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/

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

Am 25.04.2012 13:14, schrieb Jeff King:

On Wed, Apr 25, 2012 at 12:59:28AM -0700, Michael Mueller wrote:

quoted

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é

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

Signed-off-by: Rene Scharfe <redacted>
---
 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

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 <redacted>
---
 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

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

Signed-off-by: Rene Scharfe <redacted>
---
 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