clear parsed flag when we free tree buffers

peff · gitster · commit 6e454b9a3184 · 2013-06-06T10:29:12.000-07:00
Many code paths will free a tree object's buffer and set it
to NULL after finishing with it in order to keep memory
usage down during a traversal. However, out of 8 sites that
do this, only one actually unsets the "parsed" flag back.
Those sites that don't are setting a trap for later users of
the tree object; even after calling parse_tree, the buffer
will remain NULL, causing potential segfaults.

It is not known whether this is triggerable in the current
code. Most commands do not do an in-memory traversal
followed by actually using the objects again. However, it
does not hurt to be safe for future callers.

In most cases, we can abstract this out to a
"free_tree_buffer" helper. However, there are two
exceptions:

  1. The fsck code relies on the parsed flag to know that we
     were able to parse the object at one point. We can
     switch this to using a flag in the "flags" field.

  2. The index-pack code sets the buffer to NULL but does
     not free it (it is freed by a caller). We should still
     unset the parsed flag here, but we cannot use our
     helper, as we do not want to free the buffer.

Signed-off-by: Jeff King &lt;peff@peff.net&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/builtin/fsck.c b/builtin/fsck.c
@@ -16,6 +16,7 @@
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
+#define HAS_OBJ   0x0004
 
 static int show_root;
 static int show_tags;
@@ -101,7 +102,7 @@ static int mark_object(struct object *obj, int type, void *data)
 	if (obj->flags & REACHABLE)
 		return 0;
 	obj->flags |= REACHABLE;
-	if (!obj->parsed) {
+	if (!(obj->flags & HAS_OBJ)) {
 		if (parent && !has_sha1_file(obj->sha1)) {
 			printf("broken link from %7s %s\n",
 				 typename(parent->type), sha1_to_hex(parent->sha1));
@@ -127,16 +128,13 @@ static int traverse_one_object(struct object *obj)
 	struct tree *tree = NULL;
 
 	if (obj->type == OBJ_TREE) {
-		obj->parsed = 0;
 		tree = (struct tree *)obj;
 		if (parse_tree(tree) < 0)
 			return 1; /* error already displayed */
 	}
 	result = fsck_walk(obj, mark_object, obj);
-	if (tree) {
-		free(tree->buffer);
-		tree->buffer = NULL;
-	}
+	if (tree)
+		free_tree_buffer(tree);
 	return result;
 }
 
@@ -178,7 +176,7 @@ static void check_reachable_object(struct object *obj)
 	 * except if it was in a pack-file and we didn't
 	 * do a full fsck
 	 */
-	if (!obj->parsed) {
+	if (!(obj->flags & HAS_OBJ)) {
 		if (has_sha1_pack(obj->sha1))
 			return; /* it is in pack - forget about it */
 		printf("missing %s %s\n", typename(obj->type), sha1_to_hex(obj->sha1));
@@ -306,8 +304,7 @@ static int fsck_obj(struct object *obj)
 	if (obj->type == OBJ_TREE) {
 		struct tree *item = (struct tree *) obj;
 
-		free(item->buffer);
-		item->buffer = NULL;
+		free_tree_buffer(item);
 	}
 
 	if (obj->type == OBJ_COMMIT) {
@@ -340,6 +337,7 @@ static int fsck_sha1(const unsigned char *sha1)
 		return error("%s: object corrupt or missing",
 			     sha1_to_hex(sha1));
 	}
+	obj->flags |= HAS_OBJ;
 	return fsck_obj(obj);
 }
 
@@ -352,6 +350,7 @@ static int fsck_obj_buffer(const unsigned char *sha1, enum object_type type,
 		errors_found |= ERROR_OBJECT;
 		return error("%s: object corrupt or missing", sha1_to_hex(sha1));
 	}
+	obj->flags = HAS_OBJ;
 	return fsck_obj(obj);
 }
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
@@ -765,6 +765,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
 			if (obj->type == OBJ_TREE) {
 				struct tree *item = (struct tree *) obj;
 				item->buffer = NULL;
+				obj->parsed = 0;
 			}
 			if (obj->type == OBJ_COMMIT) {
 				struct commit *commit = (struct commit *) obj;
diff --git a/builtin/reflog.c b/builtin/reflog.c
@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)
 			complete = 0;
 		}
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 
 	if (complete)
 		tree->object.flags |= SEEN;
diff --git a/http-push.c b/http-push.c
@@ -1330,8 +1330,7 @@ static struct object_list **process_tree(struct tree *tree,
 			break;
 		}
 
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 	return p;
 }
 
diff --git a/list-objects.c b/list-objects.c
@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,
 				     cb_data);
 	}
 	strbuf_setlen(base, baselen);
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 static void mark_edge_parents_uninteresting(struct commit *commit,
diff --git a/reachable.c b/reachable.c
@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,
 		else
 			process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 static void process_tag(struct tag *tag, struct object_array *p,
diff --git a/revision.c b/revision.c
@@ -134,8 +134,7 @@ void mark_tree_uninteresting(struct tree *tree)
 	 * We don't care about the tree any more
 	 * after it has been marked uninteresting.
 	 */
-	free(tree->buffer);
-	tree->buffer = NULL;
+	free_tree_buffer(tree);
 }
 
 void mark_parents_uninteresting(struct commit *commit)
diff --git a/tree.c b/tree.c
@@ -225,6 +225,14 @@ int parse_tree(struct tree *item)
 	return parse_tree_buffer(item, buffer, size);
 }
 
+void free_tree_buffer(struct tree *tree)
+{
+	free(tree->buffer);
+	tree->buffer = NULL;
+	tree->size = 0;
+	tree->object.parsed = 0;
+}
+
 struct tree *parse_tree_indirect(const unsigned char *sha1)
 {
 	struct object *obj = parse_object(sha1);
diff --git a/tree.h b/tree.h
@@ -16,6 +16,7 @@ struct tree *lookup_tree(const unsigned char *sha1);
 int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);
 
 int parse_tree(struct tree *tree);
+void free_tree_buffer(struct tree *tree);
 
 /* Parses and returns the tree in the given ent, chasing tags and commits. */
 struct tree *parse_tree_indirect(const unsigned char *sha1);
diff --git a/walker.c b/walker.c
@@ -56,10 +56,7 @@ static int process_tree(struct walker *walker, struct tree *tree)
 		if (!obj || process(walker, obj))
 			return -1;
 	}
-	free(tree->buffer);
-	tree->buffer = NULL;
-	tree->size = 0;
-	tree->object.parsed = 0;
+	free_tree_buffer(tree);
 	return 0;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -765,6 +765,7 @@ static void sha1_object(const void data, struct object_entry obj_entry,`
`765`	`765`	`if (obj->type == OBJ_TREE) {`
`766`	`766`	`struct tree item = (struct tree ) obj;`
`767`	`767`	`item->buffer = NULL;`
	`768`	`+ obj->parsed = 0;`
`768`	`769`	`}`
`769`	`770`	`if (obj->type == OBJ_COMMIT) {`
`770`	`771`	`struct commit commit = (struct commit ) obj;`
Original file line number	Diff line number	Diff line change
`@@ -94,8 +94,7 @@ static int tree_is_complete(const unsigned char *sha1)`
`94`	`94`	`complete = 0;`
`95`	`95`	`}`
`96`	`96`	`}`
`97`		`- free(tree->buffer);`
`98`		`- tree->buffer = NULL;`
	`97`	`+ free_tree_buffer(tree);`
`99`	`98`
`100`	`99`	`if (complete)`
`101`	`100`	`tree->object.flags \|= SEEN;`
Original file line number	Diff line number	Diff line change
`@@ -1330,8 +1330,7 @@ static struct object_list *process_tree(struct tree tree,`
`1330`	`1330`	`break;`
`1331`	`1331`	`}`
`1332`	`1332`
`1333`		`- free(tree->buffer);`
`1334`		`- tree->buffer = NULL;`
	`1333`	`+ free_tree_buffer(tree);`
`1335`	`1334`	`return p;`
`1336`	`1335`	`}`
`1337`	`1336`
Original file line number	Diff line number	Diff line change
`@@ -123,8 +123,7 @@ static void process_tree(struct rev_info *revs,`
`123`	`123`	`cb_data);`
`124`	`124`	`}`
`125`	`125`	`strbuf_setlen(base, baselen);`
`126`		`- free(tree->buffer);`
`127`		`- tree->buffer = NULL;`
	`126`	`+ free_tree_buffer(tree);`
`128`	`127`	`}`
`129`	`128`
`130`	`129`	`static void mark_edge_parents_uninteresting(struct commit *commit,`
Original file line number	Diff line number	Diff line change
`@@ -80,8 +80,7 @@ static void process_tree(struct tree *tree,`
`80`	`80`	`else`
`81`	`81`	`process_blob(lookup_blob(entry.sha1), p, &me, entry.path, cp);`
`82`	`82`	`}`
`83`		`- free(tree->buffer);`
`84`		`- tree->buffer = NULL;`
	`83`	`+ free_tree_buffer(tree);`
`85`	`84`	`}`
`86`	`85`
`87`	`86`	`static void process_tag(struct tag tag, struct object_array p,`
Original file line number	Diff line number	Diff line change
`@@ -134,8 +134,7 @@ void mark_tree_uninteresting(struct tree *tree)`
`134`	`134`	`* We don't care about the tree any more`
`135`	`135`	`* after it has been marked uninteresting.`
`136`	`136`	`*/`
`137`		`- free(tree->buffer);`
`138`		`- tree->buffer = NULL;`
	`137`	`+ free_tree_buffer(tree);`
`139`	`138`	`}`
`140`	`139`
`141`	`140`	`void mark_parents_uninteresting(struct commit *commit)`
Original file line number	Diff line number	Diff line change
`@@ -225,6 +225,14 @@ int parse_tree(struct tree *item)`
`225`	`225`	`return parse_tree_buffer(item, buffer, size);`
`226`	`226`	`}`
`227`	`227`
	`228`	`+void free_tree_buffer(struct tree *tree)`
	`229`	`+{`
	`230`	`+ free(tree->buffer);`
	`231`	`+ tree->buffer = NULL;`
	`232`	`+ tree->size = 0;`
	`233`	`+ tree->object.parsed = 0;`
	`234`	`+}`
	`235`	`+`
`228`	`236`	`struct tree parse_tree_indirect(const unsigned char sha1)`
`229`	`237`	`{`
`230`	`238`	`struct object *obj = parse_object(sha1);`
Original file line number	Diff line number	Diff line change
`@@ -56,10 +56,7 @@ static int process_tree(struct walker walker, struct tree tree)`
`56`	`56`	`if (!obj \|\| process(walker, obj))`
`57`	`57`	`return -1;`
`58`	`58`	`}`
`59`		`- free(tree->buffer);`
`60`		`- tree->buffer = NULL;`
`61`		`- tree->size = 0;`
`62`		`- tree->object.parsed = 0;`
	`59`	`+ free_tree_buffer(tree);`
`63`	`60`	`return 0;`
`64`	`61`	`}`
`65`	`62`