Merge branch 'jh/apply-free-patch'

Valgrind reports quite a lot of discarded memory inside apply.
Fix them, audit and document the buffer ownership rules.

By Junio C Hamano (8) and Jared Hance (1)
* jh/apply-free-patch:
  apply: document buffer ownership rules across functions
  apply: tighten constness of line buffer
  apply: drop unused macro
  apply: free unused fragments for submodule patch
  apply: free patch->result
  apply: release memory for fn_table
  apply: free patch->{def,old,new}_name fields
  apply: rename free_patch() to free_patch_list()
  apply: do not leak patches and fragments

builtin/apply.c

@@ -152,9 +152,14 @@ struct fragment {
 	unsigned long leading, trailing;
 	unsigned long oldpos, oldlines;
 	unsigned long newpos, newlines;
+	/*
+	 * 'patch' is usually borrowed from buf in apply_patch(),
+	 * but some codepaths store an allocated buffer.
+	 */
 	const char *patch;
+	unsigned free_patch:1,
+		rejected:1;
 	int size;
-	int rejected;
 	int linenr;
 	struct fragment *next;
 };
@@ -196,6 +201,36 @@ struct patch {
 	struct patch *next;
 };
 
+static void free_fragment_list(struct fragment *list)
+{
+	while (list) {
+		struct fragment *next = list->next;
+		if (list->free_patch)
+			free((char *)list->patch);
+		free(list);
+		list = next;
+	}
+}
+
+static void free_patch(struct patch *patch)
+{
+	free_fragment_list(patch->fragments);
+	free(patch->def_name);
+	free(patch->old_name);
+	free(patch->new_name);
+	free(patch->result);
+	free(patch);
+}
+
+static void free_patch_list(struct patch *list)
+{
+	while (list) {
+		struct patch *next = list->next;
+		free_patch(list);
+		list = next;
+	}
+}
+
 /*
  * A line in a file, len-bytes long (includes the terminating LF,
  * except for an incomplete line at the end if the file ends with
@@ -302,6 +337,11 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign
 	img->nr++;
 }
 
+/*
+ * "buf" has the file contents to be patched (read from various sources).
+ * attach it to "image" and add line-based index to it.
+ * "image" now owns the "buf".
+ */
 static void prepare_image(struct image *image, char *buf, size_t len,
 			  int prepare_linetable)
 {
@@ -353,7 +393,6 @@ static void say_patch_name(FILE *output, const char *pre,
 	fputs(post, output);
 }
 
-#define CHUNKSIZE (8192)
 #define SLOP (16)
 
 static void read_patch_file(struct strbuf *sb, int fd)
@@ -416,7 +455,7 @@ static char *squash_slash(char *name)
 	return name;
 }
 
-static char *find_name_gnu(const char *line, char *def, int p_value)
+static char *find_name_gnu(const char *line, const char *def, int p_value)
 {
 	struct strbuf name = STRBUF_INIT;
 	char *cp;
@@ -439,11 +478,7 @@ static char *find_name_gnu(const char *line, char *def, int p_value)
 		cp++;
 	}
 
-	/* name can later be freed, so we need
-	 * to memmove, not just return cp
-	 */
 	strbuf_remove(&name, 0, cp - name.buf);
-	free(def);
 	if (root)
 		strbuf_insert(&name, 0, root, root_len);
 	return squash_slash(strbuf_detach(&name, NULL));
@@ -608,8 +643,13 @@ static size_t diff_timestamp_len(const char *line, size_t len)
 	return line + len - end;
 }
 
-static char *find_name_common(const char *line, char *def, int p_value,
+static char *null_strdup(const char *s)
-				const char *end, int terminate)
+{
+	return s ? xstrdup(s) : NULL;
+}
+
+static char *find_name_common(const char *line, const char *def,
+			      int p_value, const char *end, int terminate)
 {
 	int len;
 	const char *start = NULL;
@@ -630,10 +670,10 @@ static char *find_name_common(const char *line, char *def, int p_value,
 			start = line;
 	}
 	if (!start)
-		return squash_slash(def);
+		return squash_slash(null_strdup(def));
 	len = line - start;
 	if (!len)
-		return squash_slash(def);
+		return squash_slash(null_strdup(def));
 
 	/*
 	 * Generally we prefer the shorter name, especially
@@ -644,8 +684,7 @@ static char *find_name_common(const char *line, char *def, int p_value,
 	if (def) {
 		int deflen = strlen(def);
 		if (deflen < len && !strncmp(start, def, deflen))
-			return squash_slash(def);
+			return squash_slash(xstrdup(def));
-		free(def);
 	}
 
 	if (root) {
@@ -842,8 +881,10 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 		name = find_name_traditional(first, NULL, p_value);
 		patch->old_name = name;
 	} else {
-		name = find_name_traditional(first, NULL, p_value);
+		char *first_name;
-		name = find_name_traditional(second, name, p_value);
+		first_name = find_name_traditional(first, NULL, p_value);
+		name = find_name_traditional(second, first_name, p_value);
+		free(first_name);
 		if (has_epoch_timestamp(first)) {
 			patch->is_new = 1;
 			patch->is_delete = 0;
@@ -853,7 +894,8 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 			patch->is_delete = 1;
 			patch->old_name = name;
 		} else {
-			patch->old_name = patch->new_name = name;
+			patch->old_name = name;
+			patch->new_name = xstrdup(name);
 		}
 	}
 	if (!name)
@@ -903,13 +945,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 
 static int gitdiff_oldname(const char *line, struct patch *patch)
 {
+	char *orig = patch->old_name;
 	patch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name, "old");
+	if (orig != patch->old_name)
+		free(orig);
 	return 0;
 }
 
 static int gitdiff_newname(const char *line, struct patch *patch)
 {
+	char *orig = patch->new_name;
 	patch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name, "new");
+	if (orig != patch->new_name)
+		free(orig);
 	return 0;
 }
 
@@ -928,20 +976,23 @@ static int gitdiff_newmode(const char *line, struct patch *patch)
 static int gitdiff_delete(const char *line, struct patch *patch)
 {
 	patch->is_delete = 1;
-	patch->old_name = patch->def_name;
+	free(patch->old_name);
+	patch->old_name = null_strdup(patch->def_name);
 	return gitdiff_oldmode(line, patch);
 }
 
 static int gitdiff_newfile(const char *line, struct patch *patch)
 {
 	patch->is_new = 1;
-	patch->new_name = patch->def_name;
+	free(patch->new_name);
+	patch->new_name = null_strdup(patch->def_name);
 	return gitdiff_newmode(line, patch);
 }
 
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
+	free(patch->old_name);
 	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -949,6 +1000,7 @@ static int gitdiff_copysrc(const char *line, struct patch *patch)
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
+	free(patch->new_name);
 	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -956,6 +1008,7 @@ static int gitdiff_copydst(const char *line, struct patch *patch)
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
+	free(patch->old_name);
 	patch->old_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -963,6 +1016,7 @@ static int gitdiff_renamesrc(const char *line, struct patch *patch)
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
+	free(patch->new_name);
 	patch->new_name = find_name(line, NULL, p_value ? p_value - 1 : 0, 0);
 	return 0;
 }
@@ -1044,7 +1098,7 @@ static const char *stop_at_slash(const char *line, int llen)
  * creation or deletion of an empty file.  In any of these cases,
  * both sides are the same name under a/ and b/ respectively.
  */
-static char *git_header_name(char *line, int llen)
+static char *git_header_name(const char *line, int llen)
 {
 	const char *name;
 	const char *second = NULL;
@@ -1171,7 +1225,7 @@ static char *git_header_name(char *line, int llen)
 }
 
 /* Verify that we recognize the lines following a git header */
-static int parse_git_header(char *line, int len, unsigned int size, struct patch *patch)
+static int parse_git_header(const char *line, int len, unsigned int size, struct patch *patch)
 {
 	unsigned long offset;
 
@@ -1287,7 +1341,7 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
-static void recount_diff(char *line, int size, struct fragment *fragment)
+static void recount_diff(const char *line, int size, struct fragment *fragment)
 {
 	int oldlines = 0, newlines = 0, ret = 0;
 
@@ -1341,7 +1395,7 @@ static void recount_diff(char *line, int size, struct fragment *fragment)
  * Parse a unified diff fragment header of the
  * form "@@ -a,b +c,d @@"
  */
-static int parse_fragment_header(char *line, int len, struct fragment *fragment)
+static int parse_fragment_header(const char *line, int len, struct fragment *fragment)
 {
 	int offset;
 
@@ -1355,7 +1409,7 @@ static int parse_fragment_header(char *line, int len, struct fragment *fragment)
 	return offset;
 }
 
-static int find_header(char *line, unsigned long size, int *hdrsize, struct patch *patch)
+static int find_header(const char *line, unsigned long size, int *hdrsize, struct patch *patch)
 {
 	unsigned long offset, len;
 
@@ -1403,7 +1457,8 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 				if (!patch->def_name)
 					die("git diff header lacks filename information when removing "
 					    "%d leading pathname components (line %d)" , p_value, linenr);
-				patch->old_name = patch->new_name = patch->def_name;
+				patch->old_name = xstrdup(patch->def_name);
+				patch->new_name = xstrdup(patch->def_name);
 			}
 			if (!patch->is_delete && !patch->new_name)
 				die("git diff header lacks filename information "
@@ -1466,7 +1521,7 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule)
  * between a "---" that is part of a patch, and a "---" that starts
  * the next patch is to look at the line counts..
  */
-static int parse_fragment(char *line, unsigned long size,
+static int parse_fragment(const char *line, unsigned long size,
 			  struct patch *patch, struct fragment *fragment)
 {
 	int added, deleted;
@@ -1562,7 +1617,15 @@ static int parse_fragment(char *line, unsigned long size,
 	return offset;
 }
 
-static int parse_single_patch(char *line, unsigned long size, struct patch *patch)
+/*
+ * We have seen "diff --git a/... b/..." header (or a traditional patch
+ * header).  Read hunks that belong to this patch into fragments and hang
+ * them to the given patch structure.
+ *
+ * The (fragment->patch, fragment->size) pair points into the memory given
+ * by the caller, not a copy, when we return.
+ */
+static int parse_single_patch(const char *line, unsigned long size, struct patch *patch)
 {
 	unsigned long offset = 0;
 	unsigned long oldlines = 0, newlines = 0, context = 0;
@@ -1655,6 +1718,11 @@ static char *inflate_it(const void *data, unsigned long size,
 	return out;
 }
 
+/*
+ * Read a binary hunk and return a new fragment; fragment->patch
+ * points at an allocated memory that the caller must free, so
+ * it is marked as "->free_patch = 1".
+ */
 static struct fragment *parse_binary_hunk(char **buf_p,
 					  unsigned long *sz_p,
 					  int *status_p,
@@ -1742,6 +1810,7 @@ static struct fragment *parse_binary_hunk(char **buf_p,
 
 	frag = xcalloc(1, sizeof(*frag));
 	frag->patch = inflate_it(data, hunk_size, origlen);
+	frag->free_patch = 1;
 	if (!frag->patch)
 		goto corrupt;
 	free(data);
@@ -1807,6 +1876,13 @@ static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
 	return used;
 }
 
+/*
+ * Read the patch text in "buffer" taht extends for "size" bytes; stop
+ * reading after seeing a single patch (i.e. changes to a single file).
+ * Create fragments (i.e. patch hunks) and hang them to the given patch.
+ * Return the number of bytes consumed, so that the caller can call us
+ * again for the next patch.
+ */
 static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
 {
 	int hdrsize, patchsize;
@@ -2367,6 +2443,11 @@ static void remove_last_line(struct image *img)
 	img->len -= img->line[--img->nr].len;
 }
 
+/*
+ * The change from "preimage" and "postimage" has been found to
+ * apply at applied_pos (counts in line numbers) in "img".
+ * Update "img" to remove "preimage" and replace it with "postimage".
+ */
 static void update_image(struct image *img,
 			 int applied_pos,
 			 struct image *preimage,
@@ -2438,6 +2519,11 @@ static void update_image(struct image *img,
 	img->nr = nr;
 }
 
+/*
+ * Use the patch-hunk text in "frag" to prepare two images (preimage and
+ * postimage) for the hunk.  Find lines that match "preimage" in "img" and
+ * replace the part of "img" with "postimage" text.
+ */
 static int apply_one_fragment(struct image *img, struct fragment *frag,
 			      int inaccurate_eof, unsigned ws_rule,
 			      int nth_fragment)
@@ -2728,6 +2814,12 @@ static int apply_binary_fragment(struct image *img, struct patch *patch)
 	return -1;
 }
 
+/*
+ * Replace "img" with the result of applying the binary patch.
+ * The binary patch data itself in patch->fragment is still kept
+ * but the preimage prepared by the caller in "img" is freed here
+ * or in the helper function apply_binary_fragment() this calls.
+ */
 static int apply_binary(struct image *img, struct patch *patch)
 {
 	const char *name = patch->old_name ? patch->old_name : patch->new_name;
@@ -2935,7 +3027,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 			return error("patch %s has been renamed/deleted",
 				patch->old_name);
 		}
-		/* We have a patched copy in memory use that */
+		/* We have a patched copy in memory; use that. */
 		strbuf_add(&buf, tpatch->result, tpatch->resultsize);
 	} else if (cached) {
 		if (read_file_or_gitlink(ce, &buf))
@@ -2948,7 +3040,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 				/*
 				 * There is no way to apply subproject
 				 * patch without looking at the index.
+				 * NEEDSWORK: shouldn't this be flagged
+				 * as an error???
 				 */
+				free_fragment_list(patch->fragments);
 				patch->fragments = NULL;
 			}
 		} else {
@@ -3085,10 +3180,15 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
  is_new:
 	patch->is_new = 1;
 	patch->is_delete = 0;
+	free(patch->old_name);
 	patch->old_name = NULL;
 	return 0;
 }
 
+/*
+ * Check and apply the patch in-core; leave the result in patch->result
+ * for the caller to write it out to the final destination.
+ */
 static int check_patch(struct patch *patch)
 {
 	struct stat st;
@@ -3665,16 +3765,9 @@ static void prefix_patches(struct patch *p)
 	if (!prefix || p->is_toplevel_relative)
 		return;
 	for ( ; p; p = p->next) {
-		if (p->new_name == p->old_name) {
-			char *prefixed = p->new_name;
-			prefix_one(&prefixed);
-			p->new_name = p->old_name = prefixed;
-		}
-		else {
 		prefix_one(&p->new_name);
 		prefix_one(&p->old_name);
 	}
-	}
 }
 
 #define INACCURATE_EOF	(1<<0)
@@ -3683,12 +3776,10 @@ static void prefix_patches(struct patch *p)
 static int apply_patch(int fd, const char *filename, int options)
 {
 	size_t offset;
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT; /* owns the patch text */
 	struct patch *list = NULL, **listp = &list;
 	int skipped_patch = 0;
 
-	/* FIXME - memory leak when using multiple patch files as inputs */
-	memset(&fn_table, 0, sizeof(struct string_list));
 	patch_input_file = filename;
 	read_patch_file(&buf, fd);
 	offset = 0;
@@ -3712,8 +3803,7 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
+			free_patch(patch);
-			free(patch);
 			skipped_patch++;
 		}
 		offset += nr;
@@ -3754,7 +3844,9 @@ static int apply_patch(int fd, const char *filename, int options)
 	if (summary)
 		summary_patch_list(list);
 
+	free_patch_list(list);
 	strbuf_release(&buf);
+	string_list_clear(&fn_table, 0);
 	return 0;
 }