From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 23 Apr 2012 19:52:18 +0000 (-0700)
Subject: Merge branch 'jh/apply-free-patch'
X-Git-Tag: v1.7.11-rc0~126
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/58bbace89d3e94a20faae4df0d20d57574dff6e1?ds=inline;hp=-c

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

58bbace89d3e94a20faae4df0d20d57574dff6e1
diff --combined builtin/apply.c
index 389898f133,f27f80ed76..799bb5e906
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@@ -14,7 -14,6 +14,7 @@@
  #include "builtin.h"
  #include "string-list.h"
  #include "dir.h"
 +#include "diff.h"
  #include "parse-options.h"
  
  /*
@@@ -152,9 -151,14 +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 -200,36 +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 -336,11 +337,11 @@@ static void add_line_info(struct image 
  	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 -392,6 +393,6 @@@ static void say_patch_name(FILE *output
  	fputs(post, output);
  }
  
- #define CHUNKSIZE (8192)
  #define SLOP (16)
  
  static void read_patch_file(struct strbuf *sb, int fd)
@@@ -416,7 -454,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 -477,7 +478,7 @@@
  		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 -642,13 +643,13 @@@ static size_t diff_timestamp_len(const 
  	return line + len - end;
  }
  
- static char *find_name_common(const char *line, char *def, int p_value,
- 				const char *end, int terminate)
+ static char *null_strdup(const char *s)
+ {
+ 	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 -669,10 +670,10 @@@
  			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 -683,7 +684,7 @@@
  	if (def) {
  		int deflen = strlen(def);
  		if (deflen < len && !strncmp(start, def, deflen))
- 			return squash_slash(def);
- 		free(def);
+ 			return squash_slash(xstrdup(def));
  	}
  
  	if (root) {
@@@ -842,8 -880,10 +881,10 @@@ static void parse_traditional_patch(con
  		name = find_name_traditional(first, NULL, p_value);
  		patch->old_name = name;
  	} else {
- 		name = find_name_traditional(first, NULL, p_value);
- 		name = find_name_traditional(second, name, p_value);
+ 		char *first_name;
+ 		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 -893,8 +894,8 @@@
  			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 -944,19 +945,19 @@@ static char *gitdiff_verify_name(const 
  
  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 -975,23 +976,23 @@@ static int gitdiff_newmode(const char *
  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 -999,7 +1000,7 @@@
  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 -1007,7 +1008,7 @@@
  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 -1015,7 +1016,7 @@@
  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 -1097,7 +1098,7 @@@ static const char *stop_at_slash(const 
   * 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 -1224,7 +1225,7 @@@
  }
  
  /* 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 -1340,7 +1341,7 @@@ static int parse_range(const char *line
  	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 -1394,7 +1395,7 @@@
   * 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 -1408,7 +1409,7 @@@
  	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 -1456,8 +1457,8 @@@
  				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 -1520,7 +1521,7 @@@ static void check_whitespace(const cha
   * 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 -1616,15 +1617,15 @@@
  	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 -1717,11 +1718,11 @@@ static char *inflate_it(const void *dat
  	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 -1809,7 +1810,7 @@@
  
  	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 -1875,13 +1876,13 @@@ static int parse_binary(char *buffer, u
  	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 -2442,11 +2443,11 @@@ static void remove_last_line(struct ima
  	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 -2518,11 +2519,11 @@@
  	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 -2813,12 +2814,12 @@@ static int apply_binary_fragment(struc
  	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 -3026,7 +3027,7 @@@ static int apply_data(struct patch *pat
  			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 -3039,10 +3040,10 @@@
  				/*
  				 * 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 -3179,15 +3180,15 @@@ static int check_preimage(struct patch 
   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;
@@@ -3242,7 -3341,7 +3342,7 @@@ static void stat_patch_list(struct patc
  		show_stats(patch);
  	}
  
 -	printf(" %d files changed, %d insertions(+), %d deletions(-)\n", files, adds, dels);
 +	print_stat_summary(stdout, files, adds, dels);
  }
  
  static void numstat_patch_list(struct patch *patch)
@@@ -3665,15 -3764,8 +3765,8 @@@ static void prefix_patches(struct patc
  	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);
- 		}
+ 		prefix_one(&p->new_name);
+ 		prefix_one(&p->old_name);
  	}
  }
  
@@@ -3683,12 -3775,10 +3776,10 @@@
  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 -3802,7 +3803,7 @@@
  			listp = &patch->next;
  		}
  		else {
- 			/* perhaps free it a bit better? */
- 			free(patch);
+ 			free_patch(patch);
  			skipped_patch++;
  		}
  		offset += nr;
@@@ -3754,7 -3843,9 +3844,9 @@@
  	if (summary)
  		summary_patch_list(list);
  
+ 	free_patch_list(list);
  	strbuf_release(&buf);
+ 	string_list_clear(&fn_table, 0);
  	return 0;
  }