CVE-2016-2324: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: Integer overflow in Git before 2.7.4 allows remote attackers to execute arbitrary code via a (1) long filename or (2) many nested trees, which triggers a heap-based buffer overflow.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 9.8
Published: 2016-04-08T14:59Z
Repository: https://github.com/git/git
Language: C
Files changed: builtin/pack-objects.c, builtin/rev-list.c, list-objects.c, list-objects.h, pack-bitmap-write.c, pack-bitmap.c, reachable.c, revision.c, revision.h
Commit date: 2016-02-12 12:51:17 -0800
Commit message: list-objects: pass full pathname to callbacks

When we find a blob at "a/b/c", we currently pass this to
our show_object_fn callbacks as two components: "a/b/" and
"c". Callbacks which want the full value then call
path_name(), which concatenates the two. But this is an
inefficient interface; the path is a strbuf, and we could
simply append "c" to it temporarily, then roll back the
length, without creating a new copy.

So we could improve this by teaching the callsites of
path_name() this trick (and there are only 3). But we can
also notice that no callback actually cares about the
broken-down representation, and simply pass each callback
the full path "a/b/c" as a string. The callback code becomes
even simpler, then, as we do not have to worry about freeing
an allocated buffer, nor rolling back our modification to
the strbuf.

This is theoretically less efficient, as some callbacks
would not bother to format the final path component. But in
practice this is not measurable. Since we use the same
strbuf over and over, our work to grow it is amortized, and
we really only pay to memcpy a few bytes.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Diff: diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8bbb9bd9aa..a6609f19ff 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2282,25 +2282,15 @@ static void show_commit(struct commit *commit, void *data)
 
 	if (write_bitmap_index)
 		index_commit_for_bitmap(commit);
 }
 
-static void show_object(struct object *obj,
-			struct strbuf *path, const char *last,
-			void *data)
+static void show_object(struct object *obj, const char *name, void *data)
 {
-	char *name = path_name(path, last);
-
 	add_preferred_base_object(name);
 	add_object_entry(obj->oid.hash, obj->type, name, 0);
 	obj->flags |= OBJECT_ADDED;
-
-	/*
-	 * We will have generated the hash from the name,
-	 * but not saved a pointer to it - we can free it
-	 */
-	free((char *)name);
 }
 
 static void show_edge(struct commit *commit)
 {
 	add_preferred_base(commit->object.oid.hash);
@@ -2478,12 +2468,11 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
 	traverse_bitmap_commit_list(&add_object_entry_from_bitmap);
 	return 0;
 }
 
 static void record_recent_object(struct object *obj,
-				 struct strbuf *path,
-				 const char *last,
+				 const char *name,
 				 void *data)
 {
 	sha1_array_append(&recent_objects, obj->oid.hash);
 }
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index a92c3ca852..275da0d647 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -175,30 +175,26 @@ static void finish_commit(struct commit *commit, void *data)
 		commit->parents = NULL;
 	}
 	free_commit_buffer(commit);
 }
 
-static void finish_object(struct object *obj,
-			  struct strbuf *path, const char *name,
-			  void *cb_data)
+static void finish_object(struct object *obj, const char *name, void *cb_data)
 {
 	struct rev_list_info *info = cb_data;
 	if (obj->type == OBJ_BLOB && !has_object_file(&obj->oid))
 		die("missing blob object '%s'", oid_to_hex(&obj->oid));
 	if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
 		parse_object(obj->oid.hash);
 }
 
-static void show_object(struct object *obj,
-			struct strbuf *path, const char *component,
-			void *cb_data)
+static void show_object(struct object *obj, const char *name, void *cb_data)
 {
 	struct rev_list_info *info = cb_data;
-	finish_object(obj, path, component, cb_data);
+	finish_object(obj, name, cb_data);
 	if (info->flags & REV_LIST_QUIET)
 		return;
-	show_object_with_name(stdout, obj, path, component);
+	show_object_with_name(stdout, obj, name);
 }
 
 static void show_edge(struct commit *commit)
 {
 	printf("-%s\n", oid_to_hex(&commit->object.oid));
diff --git a/list-objects.c b/list-objects.c
index 43977665d3..917cc5d7c9 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -14,19 +14,24 @@ static void process_blob(struct rev_info *revs,
 			 struct strbuf *path,
 			 const char *name,
 			 void *cb_data)
 {
 	struct object *obj = &blob->object;
+	size_t pathlen;
 
 	if (!revs->blob_objects)
 		return;
 	if (!obj)
 		die("bad blob object");
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
 	obj->flags |= SEEN;
-	show(obj, path, name, cb_data);
+
+	pathlen = path->len;
+	strbuf_addstr(path, name);
+	show(obj, path->buf, cb_data);
+	strbuf_setlen(path, pathlen);
 }
 
 /*
  * Processing a gitlink entry currently does nothing, since
  * we do not recurse into the subproject.
@@ -84,13 +89,12 @@ static void process_tree(struct rev_info *revs,
 			return;
 		die("bad tree object %s", oid_to_hex(&obj->oid));
 	}
 
 	obj->flags |= SEEN;
-	show(obj, base, name, cb_data);
-
 	strbuf_addstr(base, name);
+	show(obj, base->buf, cb_data);
 	if (base->len)
 		strbuf_addch(base, '/');
 
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
@@ -205,11 +209,11 @@ void traverse_commit_list(struct rev_info *revs,
 		const char *path = pending->path;
 		if (obj->flags & (UNINTERESTING | SEEN))
 			continue;
 		if (obj->type == OBJ_TAG) {
 			obj->flags |= SEEN;
-			show_object(obj, NULL, name, data);
+			show_object(obj, name, data);
 			continue;
 		}
 		if (!path)
 			path = "";
 		if (obj->type == OBJ_TREE) {
@@ -217,11 +221,11 @@ void traverse_commit_list(struct rev_info *revs,
 				     &base, path, data);
 			continue;
 		}
 		if (obj->type == OBJ_BLOB) {
 			process_blob(revs, (struct blob *)obj, show_object,
-				     NULL, path, data);
+				     &base, path, data);
 			continue;
 		}
 		die("unknown pending object %s (%s)",
 		    oid_to_hex(&obj->oid), name);
 	}
diff --git a/list-objects.h b/list-objects.h
index 69c4c7de6e..0cebf8585c 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -1,10 +1,10 @@
 #ifndef LIST_OBJECTS_H
 #define LIST_OBJECTS_H
 
 typedef void (*show_commit_fn)(struct commit *, void *);
-typedef void (*show_object_fn)(struct object *, struct strbuf *, const char *, void *);
+typedef void (*show_object_fn)(struct object *, const char *, void *);
 void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
 
 typedef void (*show_edge_fn)(struct commit *);
 void mark_edges_uninteresting(struct rev_info *, show_edge_fn);
 
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 65ed3425b3..c30bcd06cb 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -146,12 +146,11 @@ static uint32_t find_object_pos(const unsigned char *sha1)
 	}
 
 	return entry->in_pack_pos;
 }
 
-static void show_object(struct object *object, struct strbuf *path,
-			const char *last, void *data)
+static void show_object(struct object *object, const char *name, void *data)
 {
 	struct bitmap *base = data;
 	bitmap_set(base, find_object_pos(object->oid.hash));
 	mark_as_seen(object);
 }
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 51f790e7ce..b949e51716 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -412,23 +412,19 @@ static int ext_index_add_object(struct object *object, const char *name)
 	}
 
 	return bitmap_pos + bitmap_git.pack->num_objects;
 }
 
-static void show_object(struct object *object, struct strbuf *path,
-			const char *last, void *data)
+static void show_object(struct object *object, const char *name, void *data)
 {
 	struct bitmap *base = data;
 	int bitmap_pos;
 
 	bitmap_pos = bitmap_position(object->oid.hash);
 
-	if (bitmap_pos < 0) {
-		char *name = path_name(path, last);
+	if (bitmap_pos < 0)
 		bitmap_pos = ext_index_add_object(object, name);
-		free(name);
-	}
 
 	bitmap_set(base, bitmap_pos);
 }
 
 static void show_commit(struct commit *commit, void *data)
@@ -892,13 +888,12 @@ struct bitmap_test_data {
 	struct bitmap *base;
 	struct progress *prg;
 	size_t seen;
 };
 
-static void test_show_object(struct object *object,
-			     struct strbuf *path,
-			     const char *last, void *data)
+static void test_show_object(struct object *object, const char *name,
+			     void *data)
 {
 	struct bitmap_test_data *tdata = data;
 	int bitmap_pos;
 
 	bitmap_pos = bitmap_position(object->oid.hash);
diff --git a/reachable.c b/reachable.c
index e60f08d2f0..ed35201896 100644
--- a/reachable.c
+++ b/reachable.c
@@ -41,19 +41,18 @@ static int add_one_ref(const char *path, const struct object_id *oid,
 
 /*
  * The traversal will have already marked us as SEEN, so we
  * only need to handle any progress reporting here.
  */
-static void mark_object(struct object *obj, struct strbuf *path,
-			const char *name, void *data)
+static void mark_object(struct object *obj, const char *name, void *data)
 {
 	update_progress(data);
 }
 
 static void mark_commit(struct commit *c, void *data)
 {
-	mark_object(&c->object, NULL, NULL, data);
+	mark_object(&c->object, NULL, data);
 }
 
 struct recent_data {
 	struct rev_info *revs;
 	unsigned long timestamp;
diff --git a/revision.c b/revision.c
index 3c84781816..82f3ca44b3 100644
--- a/revision.c
+++ b/revision.c
@@ -23,31 +23,18 @@
 volatile show_early_output_fn_t show_early_output;
 
 static const char *term_bad;
 static const char *term_good;
 
-char *path_name(struct strbuf *path, const char *name)
+void show_object_with_name(FILE *out, struct object *obj, const char *name)
 {
-	struct strbuf ret = STRBUF_INIT;
-	if (path)
-		strbuf_addbuf(&ret, path);
-	strbuf_addstr(&ret, name);
-	return strbuf_detach(&ret, NULL);
-}
-
-void show_object_with_name(FILE *out, struct object *obj,
-			   struct strbuf *path, const char *component)
-{
-	char *name = path_name(path, component);
-	char *p;
+	const char *p;
 
 	fprintf(out, "%s ", oid_to_hex(&obj->oid));
 	for (p = name; *p && *p != '\n'; p++)
 		fputc(*p, out);
 	fputc('\n', out);
-
-	free(name);
 }
 
 static void mark_blob_uninteresting(struct blob *blob)
 {
 	if (!blob)
diff --git a/revision.h b/revision.h
index 7beab15248..dca0d38171 100644
--- a/revision.h
+++ b/revision.h
@@ -257,12 +257,11 @@ extern void put_revision_mark(const struct rev_info *revs,
 extern void mark_parents_uninteresting(struct commit *commit);
 extern void mark_tree_uninteresting(struct tree *tree);
 
 char *path_name(struct strbuf *path, const char *name);
 
-extern void show_object_with_name(FILE *, struct object *,
-				  struct strbuf *, const char *);
+extern void show_object_with_name(FILE *, struct object *, const char *);
 
 extern void add_pending_object(struct rev_info *revs,
 			       struct object *obj, const char *name);
 extern void add_pending_sha1(struct rev_info *revs,
 			     const char *name, const unsigned char *sha1,

Vulnerable code: static void show_object(struct object *obj,
			struct strbuf *path, const char *last,
			void *data)
	char *name = path_name(path, last);


	/*
	 * We will have generated the hash from the name,
	 * but not saved a pointer to it - we can free it
	 */
	free((char *)name);
				 struct strbuf *path,
				 const char *last,
static void finish_object(struct object *obj,
			  struct strbuf *path, const char *name,
			  void *cb_data)
static void show_object(struct object *obj,
			struct strbuf *path, const char *component,
			void *cb_data)
	finish_object(obj, path, component, cb_data);
	show_object_with_name(stdout, obj, path, component);
	show(obj, path, name, cb_data);
	show(obj, base, name, cb_data);

			show_object(obj, NULL, name, data);
				     NULL, path, data);
typedef void (*show_object_fn)(struct object *, struct strbuf *, const char *, void *);
static void show_object(struct object *object, struct strbuf *path,
			const char *last, void *data)
static void show_object(struct object *object, struct strbuf *path,
			const char *last, void *data)
	if (bitmap_pos < 0) {
		char *name = path_name(path, last);
		free(name);
	}
static void test_show_object(struct object *object,
			     struct strbuf *path,
			     const char *last, void *data)
static void mark_object(struct object *obj, struct strbuf *path,
			const char *name, void *data)
	mark_object(&c->object, NULL, NULL, data);
char *path_name(struct strbuf *path, const char *name)
	struct strbuf ret = STRBUF_INIT;
	if (path)
		strbuf_addbuf(&ret, path);
	strbuf_addstr(&ret, name);
	return strbuf_detach(&ret, NULL);
}

void show_object_with_name(FILE *out, struct object *obj,
			   struct strbuf *path, const char *component)
{
	char *name = path_name(path, component);
	char *p;

	free(name);
extern void show_object_with_name(FILE *, struct object *,
				  struct strbuf *, const char *);
Fixed code: static void show_object(struct object *obj, const char *name, void *data)
				 const char *name,
static void finish_object(struct object *obj, const char *name, void *cb_data)
static void show_object(struct object *obj, const char *name, void *cb_data)
	finish_object(obj, name, cb_data);
	show_object_with_name(stdout, obj, name);
	size_t pathlen;

	pathlen = path->len;
	strbuf_addstr(path, name);
	show(obj, path->buf, cb_data);
	strbuf_setlen(path, pathlen);
	show(obj, base->buf, cb_data);
			show_object(obj, name, data);
				     &base, path, data);
typedef void (*show_object_fn)(struct object *, const char *, void *);
static void show_object(struct object *object, const char *name, void *data)
static void show_object(struct object *object, const char *name, void *data)
	if (bitmap_pos < 0)
static void test_show_object(struct object *object, const char *name,
			     void *data)
static void mark_object(struct object *obj, const char *name, void *data)
	mark_object(&c->object, NULL, data);
void show_object_with_name(FILE *out, struct object *obj, const char *name)
	const char *p;
extern void show_object_with_name(FILE *, struct object *, const char *);