Merge branch 'ma/leakplugs'

gitster · gitster · commit 69c54c72845e · 2017-09-29T11:23:43.000+09:00
Memory leaks in various codepaths have been plugged.

* ma/leakplugs:
  pack-bitmap[-write]: use `object_array_clear()`, don't leak
  object_array: add and use `object_array_pop()`
  object_array: use `object_array_clear()`, not `free()`
  leak_pending: use `object_array_clear()`, not `free()`
  commit: fix memory leak in `reduce_heads()`
  builtin/commit: fix memory leak in `prepare_index()`
diff --git a/bisect.c b/bisect.c
@@ -826,7 +826,8 @@ static int check_ancestors(const char *prefix)
 
 	/* Clean up objects used, as they will be reused. */
 	clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);
-	free(pending_copy.objects);
+
+	object_array_clear(&pending_copy);
 
 	return res;
 }
diff --git a/builtin/checkout.c b/builtin/checkout.c
@@ -797,18 +797,25 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)
 	for_each_ref(add_pending_uninteresting_ref, &revs);
 	add_pending_oid(&revs, "HEAD", &new->object.oid, UNINTERESTING);
 
+	/* Save pending objects, so they can be cleaned up later. */
 	refs = revs.pending;
 	revs.leak_pending = 1;
 
+	/*
+	 * prepare_revision_walk (together with .leak_pending = 1) makes us
+	 * the sole owner of the list of pending objects.
+	 */
 	if (prepare_revision_walk(&revs))
 		die(_("internal error in revision walk"));
 	if (!(old->object.flags & UNINTERESTING))
 		suggest_reattach(old, &revs);
 	else
 		describe_detached_head(_("Previous HEAD position was"), old);
 
+	/* Clean up objects used, as they will be reused. */
 	clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);
-	free(refs.objects);
+
+	object_array_clear(&refs);
 }
 
 static int switch_branches(const struct checkout_opts *opts,
diff --git a/builtin/commit.c b/builtin/commit.c
@@ -335,7 +335,7 @@ static void refresh_cache_or_die(int refresh_flags)
 static const char *prepare_index(int argc, const char **argv, const char *prefix,
 				 const struct commit *current_head, int is_status)
 {
-	struct string_list partial;
+	struct string_list partial = STRING_LIST_INIT_DUP;
 	struct pathspec pathspec;
 	int refresh_flags = REFRESH_QUIET;
 	const char *ret;
@@ -380,7 +380,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			warning(_("Failed to update main cache tree"));
 
 		commit_style = COMMIT_NORMAL;
-		return get_lock_file_path(&index_lock);
+		ret = get_lock_file_path(&index_lock);
+		goto out;
 	}
 
 	/*
@@ -403,7 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 		if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
 			die(_("unable to write new_index file"));
 		commit_style = COMMIT_NORMAL;
-		return get_lock_file_path(&index_lock);
+		ret = get_lock_file_path(&index_lock);
+		goto out;
 	}
 
 	/*
@@ -429,7 +431,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			rollback_lock_file(&index_lock);
 		}
 		commit_style = COMMIT_AS_IS;
-		return get_index_file();
+		ret = get_index_file();
+		goto out;
 	}
 
 	/*
@@ -460,7 +463,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			die(_("cannot do a partial commit during a cherry-pick."));
 	}
 
-	string_list_init(&partial, 1);
 	if (list_paths(&partial, !current_head ? NULL : "HEAD", prefix, &pathspec))
 		exit(1);
 
@@ -490,6 +492,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	discard_cache();
 	ret = get_lock_file_path(&false_lock);
 	read_cache_from(ret);
+out:
+	string_list_clear(&partial, 0);
+	clear_pathspec(&pathspec);
 	return ret;
 }
 
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
@@ -650,11 +650,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs,
 {
 	struct commit *commit;
 	while (commits->nr) {
-		commit = (struct commit *)commits->objects[commits->nr - 1].item;
+		commit = (struct commit *)object_array_pop(commits);
 		if (has_unshown_parent(commit))
 			return;
 		handle_commit(commit, revs, paths_of_changed_objects);
-		commits->nr--;
 	}
 }
 
diff --git a/builtin/fsck.c b/builtin/fsck.c
@@ -182,12 +182,7 @@ static int traverse_reachable(void)
 	if (show_progress)
 		progress = start_delayed_progress(_("Checking connectivity"), 0);
 	while (pending.nr) {
-		struct object_array_entry *entry;
-		struct object *obj;
-
-		entry = pending.objects + --pending.nr;
-		obj = entry->item;
-		result |= traverse_one_object(obj);
+		result |= traverse_one_object(object_array_pop(&pending));
 		display_progress(progress, ++nr);
 	}
 	stop_progress(&progress);
diff --git a/builtin/reflog.c b/builtin/reflog.c
@@ -126,7 +126,7 @@ static int commit_is_complete(struct commit *commit)
 		struct commit *c;
 		struct commit_list *parent;
 
-		c = (struct commit *)study.objects[--study.nr].item;
+		c = (struct commit *)object_array_pop(&study);
 		if (!c->object.parsed && !parse_object(&c->object.oid))
 			c->object.flags |= INCOMPLETE;
 
@@ -182,8 +182,8 @@ static int commit_is_complete(struct commit *commit)
 			found.objects[i].item->flags |= SEEN;
 	}
 	/* free object arrays */
-	free(study.objects);
-	free(found.objects);
+	object_array_clear(&study);
+	object_array_clear(&found);
 	return !is_incomplete;
 }
 
diff --git a/bundle.c b/bundle.c
@@ -157,9 +157,14 @@ int verify_bundle(struct bundle_header *header, int verbose)
 	req_nr = revs.pending.nr;
 	setup_revisions(2, argv, &revs, NULL);
 
+	/* Save pending objects, so they can be cleaned up later. */
 	refs = revs.pending;
 	revs.leak_pending = 1;
 
+	/*
+	 * prepare_revision_walk (together with .leak_pending = 1) makes us
+	 * the sole owner of the list of pending objects.
+	 */
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 
@@ -176,8 +181,10 @@ int verify_bundle(struct bundle_header *header, int verbose)
 				refs.objects[i].name);
 		}
 
+	/* Clean up objects used, as they will be reused. */
 	clear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);
-	free(refs.objects);
+
+	object_array_clear(&refs);
 
 	if (verbose) {
 		struct ref_list *r;
diff --git a/commit.c b/commit.c
@@ -1086,6 +1086,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 	num_head = remove_redundant(array, num_head);
 	for (i = 0; i < num_head; i++)
 		tail = &commit_list_insert(array[i], tail)->next;
+	free(array);
 	return result;
 }
 
diff --git a/diff-lib.c b/diff-lib.c
@@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,
 	rev.diffopt.flags |= diff_flags;
 	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
 	run_diff_index(&rev, 1);
-	if (rev.pending.alloc)
-		free(rev.pending.objects);
+	object_array_clear(&rev.pending);
 	return (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);
 }
diff --git a/object.c b/object.c
@@ -353,6 +353,19 @@ static void object_array_release_entry(struct object_array_entry *ent)
 	free(ent->path);
 }
 
+struct object *object_array_pop(struct object_array *array)
+{
+	struct object *ret;
+
+	if (!array->nr)
+		return NULL;
+
+	ret = array->objects[array->nr - 1].item;
+	object_array_release_entry(&array->objects[array->nr - 1]);
+	array->nr--;
+	return ret;
+}
+
 void object_array_filter(struct object_array *array,
 			 object_array_each_func_t want, void *cb_data)
 {
diff --git a/object.h b/object.h
@@ -116,6 +116,14 @@ int object_list_contains(struct object_list *list, struct object *obj);
 void add_object_array(struct object *obj, const char *name, struct object_array *array);
 void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
 
+/*
+ * Returns NULL if the array is empty. Otherwise, returns the last object
+ * after removing its entry from the array. Other resources associated
+ * with that object are left in an unspecified state and should not be
+ * examined.
+ */
+struct object *object_array_pop(struct object_array *array);
+
 typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);
 
 /*
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
@@ -297,9 +297,7 @@ void bitmap_writer_build(struct packing_data *to_pack)
 
 			traverse_commit_list(&revs, show_commit, show_object, base);
 
-			revs.pending.nr = 0;
-			revs.pending.alloc = 0;
-			revs.pending.objects = NULL;
+			object_array_clear(&revs.pending);
 
 			stored->bitmap = bitmap_to_ewah(base);
 			need_reset = 0;
diff --git a/pack-bitmap.c b/pack-bitmap.c
@@ -654,8 +654,6 @@ static int in_bitmapped_pack(struct object_list *roots)
 int prepare_bitmap_walk(struct rev_info *revs)
 {
 	unsigned int i;
-	unsigned int pending_nr = revs->pending.nr;
-	struct object_array_entry *pending_e = revs->pending.objects;
 
 	struct object_list *wants = NULL;
 	struct object_list *haves = NULL;
@@ -670,8 +668,8 @@ int prepare_bitmap_walk(struct rev_info *revs)
 			return -1;
 	}
 
-	for (i = 0; i < pending_nr; ++i) {
-		struct object *object = pending_e[i].item;
+	for (i = 0; i < revs->pending.nr; ++i) {
+		struct object *object = revs->pending.objects[i].item;
 
 		if (object->type == OBJ_NONE)
 			parse_object_or_die(&object->oid, NULL);
@@ -715,9 +713,7 @@ int prepare_bitmap_walk(struct rev_info *revs)
 	if (!bitmap_git.loaded && load_pack_bitmap() < 0)
 		return -1;
 
-	revs->pending.nr = 0;
-	revs->pending.alloc = 0;
-	revs->pending.objects = NULL;
+	object_array_clear(&revs->pending);
 
 	if (haves) {
 		revs->ignore_missing_links = 1;
diff --git a/revision.h b/revision.h
@@ -150,6 +150,17 @@ struct rev_info {
 			date_mode_explicit:1,
 			preserve_subject:1;
 	unsigned int	disable_stdin:1;
+	/*
+	 * Set `leak_pending` to prevent `prepare_revision_walk()` from clearing
+	 * the array of pending objects (`pending`). It will still forget about
+	 * the array and its entries, so they really are leaked. This can be
+	 * useful if the `struct object_array` `pending` is copied before
+	 * calling `prepare_revision_walk()`. By setting `leak_pending`, you
+	 * effectively claim ownership of the old array, so you should most
+	 * likely call `object_array_clear(&pending_copy)` once you are done.
+	 * Observe that this is about ownership of the array and its entries,
+	 * not the commits referenced by those entries.
+	 */
 	unsigned int	leak_pending:1;
 	/* --show-linear-break */
 	unsigned int	track_linear:1,
diff --git a/shallow.c b/shallow.c
@@ -99,7 +99,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 				cur_depth = 0;
 			} else {
 				commit = (struct commit *)
-					stack.objects[--stack.nr].item;
+					object_array_pop(&stack);
 				cur_depth = *(int *)commit->util;
 			}
 		}
diff --git a/submodule.c b/submodule.c
@@ -1685,7 +1685,7 @@ static int find_first_merges(struct object_array *result, const char *path,
 			add_object_array(merges.objects[i].item, NULL, result);
 	}
 
-	free(merges.objects);
+	object_array_clear(&merges);
 	return result->nr;
 }
 
@@ -1790,7 +1790,7 @@ int merge_submodule(struct object_id *result, const char *path,
 			print_commit((struct commit *) merges.objects[i].item);
 	}
 
-	free(merges.objects);
+	object_array_clear(&merges);
 	return 0;
 }
 
diff --git a/upload-pack.c b/upload-pack.c
@@ -888,7 +888,7 @@ static void receive_needs(void)
 		}
 
 	shallow_nr += shallows.nr;
-	free(shallows.objects);
+	object_array_clear(&shallows);
 }
 
 /* return non-zero if the ref is hidden, otherwise 0 */

Original file line number	Diff line number	Diff line change
`@@ -826,7 +826,8 @@ static int check_ancestors(const char *prefix)`
`826`	`826`
`827`	`827`	`/* Clean up objects used, as they will be reused. */`
`828`	`828`	`clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);`
`829`		`- free(pending_copy.objects);`
	`829`	`+`
	`830`	`+ object_array_clear(&pending_copy);`
`830`	`831`
`831`	`832`	`return res;`
`832`	`833`	`}`
Original file line number	Diff line number	Diff line change
`@@ -650,11 +650,10 @@ static void handle_tail(struct object_array commits, struct rev_info revs,`
`650`	`650`	`{`
`651`	`651`	`struct commit *commit;`
`652`	`652`	`while (commits->nr) {`
`653`		`- commit = (struct commit *)commits->objects[commits->nr - 1].item;`
	`653`	`+ commit = (struct commit *)object_array_pop(commits);`
`654`	`654`	`if (has_unshown_parent(commit))`
`655`	`655`	`return;`
`656`	`656`	`handle_commit(commit, revs, paths_of_changed_objects);`
`657`		`- commits->nr--;`
`658`	`657`	`}`
`659`	`658`	`}`
`660`	`659`
Original file line number	Diff line number	Diff line change
`@@ -1086,6 +1086,7 @@ struct commit_list reduce_heads(struct commit_list heads)`
`1086`	`1086`	`num_head = remove_redundant(array, num_head);`
`1087`	`1087`	`for (i = 0; i < num_head; i++)`
`1088`	`1088`	`tail = &commit_list_insert(array[i], tail)->next;`
	`1089`	`+ free(array);`
`1089`	`1090`	`return result;`
`1090`	`1091`	`}`
`1091`	`1092`
Original file line number	Diff line number	Diff line change
`@@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,`
`549`	`549`	`rev.diffopt.flags \|= diff_flags;`
`550`	`550`	`rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;`
`551`	`551`	`run_diff_index(&rev, 1);`
`552`		`- if (rev.pending.alloc)`
`553`		`- free(rev.pending.objects);`
	`552`	`+ object_array_clear(&rev.pending);`
`554`	`553`	`return (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);`
`555`	`554`	`}`