threads / patch / 29873

v3, 3 partsFix documented fixme's throughout

Subject: [PATCH v3 0/3] Fix documented fixme's throughout

## tl;dr

9 messages between Mar 7, 2012 and Mar 21, 2012. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Jared Hance· Mar 7, 2012, 22:21 UTC · lore

A few patches that (hopefully) don't change the behavior of git except to rectify a memory error. Also, this should ever so slightly help with the parallelism of git (a GSoC proposal). All of these were found with commented FIXME and git grep.

Jared Hance (3):
  Fix memory leak in apply_patch in apply.c.
  Add threaded versions of functions in symlinks.c.
  Use startup_info->prefix rather than prefix.
 builtin/apply.c |   31 ++++++++++++++++++++++++++++---
 cache.h         |    4 +++-
 git.c           |    2 +-
 symlinks.c      |   28 ++++++++++++++++++++++++++--
 trace.c         |   10 +++++-----
 5 files changed, 63 insertions(+), 12 deletions(-)
-- 
Minor style changes and move the most controversial commit to the end of the
patch series.

Sorry, accidently sent out an old saved version of this just a bit before.

1.7.3.4
Jared Hance· Mar 7, 2012, 22:21 UTC · re: Jared Hance · lore

[PATCH v3 1/3] Fix memory leak in apply_patch in apply.c.

In the while loop inside apply_patch, patch is dynamically allocated with a calloc. However, only unused patches are actually free'd; the rest are left in a memory leak. Since a list is actively built up consisting of the used patches, they can simply be iterated and free'd at the end of the function.

In addition, the list of fragments should be free'd. To fix this, the utility function free_patch has been implemented. It loops over the entire patch list, and in each patch, loops over the fragment list, freeing the fragments, followed by the patch in the list. It frees both patch and patch->next.

The main caveat is that the text in a fragment, ie, patch->fragments->patch, may or may not need to be free'd. The text is dynamically allocated and needs to be freed iff the patch is a binary patch, as allocation occurs in inflate_it.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 builtin/apply.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)
Show changes to builtin/apply.c +28 −3
diff --git a/builtin/apply.c b/builtin/apply.c
index 389898f..4c6b278 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -153,6 +153,7 @@ struct fragment {
 	unsigned long oldpos, oldlines;
 	unsigned long newpos, newlines;
 	const char *patch;
+	unsigned int free_patch:1;
 	int size;
 	int rejected;
 	int linenr;
@@ -196,6 +197,29 @@ struct patch {
 	struct patch *next;
 };
 
+static void free_patch(struct patch *patch)
+{
+	while (patch != NULL) {
+		struct patch *patch_next;
+		struct fragment *fragment;
+
+		patch_next = patch->next;
+
+		fragment = patch->fragments;
+		while (fragment != NULL) {
+			struct fragment *fragment_next = fragment->next;
+			if (fragment->patch != NULL && fragment->free_patch) {
+				free((void*) fragment->patch);
+			}
+			free(fragment);
+			fragment = fragment_next;
+		}
+
+		free(patch);
+		patch = patch_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
@@ -1742,6 +1766,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);
@@ -3687,7 +3712,6 @@ static int apply_patch(int fd, const char *filename, int options)
 	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);
@@ -3712,8 +3736,7 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
-			free(patch);
+			free_patch(patch);
 			skipped_patch++;
 		}
 		offset += nr;
@@ -3754,6 +3777,8 @@ static int apply_patch(int fd, const char *filename, int options)
 	if (summary)
 		summary_patch_list(list);
 
+	free_patch(list);
+
 	strbuf_release(&buf);
 	return 0;
 }

-- 
1.7.3.4
Junio C Hamano· Mar 7, 2012, 23:39 UTC · re: Jared Hance · lore

Re: [PATCH v3 1/3] Fix memory leak in apply_patch in apply.c.

Jared Hance <jaredhance@gmail.com> writes:
Show 6 quoted lines
> In the while loop inside apply_patch, patch is dynamically allocated
> with a calloc. However, only unused patches are actually free'd; the
> rest are left in a memory leak. Since a list is actively built up
> consisting of the used patches, they can simply be iterated and free'd
> at the end of the function.
> ...
Thanks.

This more-or-less looks good modulo minor style issues. We might also want to make rejected a one-bit bitfield that sits next to the new free_patch field to share the same word, but that is a separate topic.

Will queue.
Junio C Hamano· Mar 21, 2012, 22:18 UTC · re: Jared Hance · lore

[PATCH 1/2] apply: free patch->{def,old,new}_name fields

These were all allocated in the heap by parsing the header parts of the patch, but we did not bother to free them. Some used to share the memory (e.g. copying def_name to old_name) so this is not just the matter of adding three calls to free().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c |   65 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 26 deletions(-)
Show changes to builtin/apply.c +39 −26
diff --git a/builtin/apply.c b/builtin/apply.c
index 427c263..5d03e50 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -209,6 +209,9 @@ static void free_patch(struct patch *patch)
 			free(fragment);
 			fragment = fragment_next;
 		}
+		free(patch->def_name);
+		free(patch->old_name);
+		free(patch->new_name);
 		free(patch);
 		patch = patch_next;
 	}
@@ -434,7 +437,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;
@@ -457,11 +460,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));
@@ -626,8 +625,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,
-				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;
@@ -648,10 +652,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
@@ -662,8 +666,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);
-		free(def);
+			return squash_slash(xstrdup(def));
 	}
 
 	if (root) {
@@ -860,8 +863,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);
-		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;
@@ -871,7 +876,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)
@@ -921,13 +927,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;
 }
 
@@ -946,20 +958,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;
 }
@@ -967,6 +982,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;
 }
@@ -974,6 +990,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;
 }
@@ -981,6 +998,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;
 }
@@ -1421,7 +1439,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 "
@@ -3104,6 +3123,7 @@ 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;
 }
@@ -3684,15 +3704,8 @@ 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);
-		}
+		prefix_one(&p->new_name);
+		prefix_one(&p->old_name);
 	}
 }
 

-- 
1.7.10.rc1.76.g1a8310
Junio C Hamano· Mar 21, 2012, 22:21 UTC · re: Junio C Hamano · lore

[PATCH 2/2] apply: free patch->result

This is by far the largest piece of data, much larger than the patch and fragment structures or the three name fields in the patch structure.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
I have not finished auditing all the codepaths, so this change needs to be
eyeballed carefully. We may be pointing an unfreeable piece of memory or a
piece of memory that belong to other structures that are going to be freed
otherwise.
 builtin/apply.c |    1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/apply.c +1 −0
diff --git a/builtin/apply.c b/builtin/apply.c
index 5d03e50..c919db3 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -212,6 +212,7 @@ static void free_patch(struct patch *patch)
 		free(patch->def_name);
 		free(patch->old_name);
 		free(patch->new_name);
+		free(patch->result);
 		free(patch);
 		patch = patch_next;
 	}

-- 
1.7.10.rc1.76.g1a8310
Jared Hance· Mar 7, 2012, 22:21 UTC · re: Jared Hance · lore

[PATCH v3 2/3] Add threaded versions of functions in symlinks.c.

check_leading_path and has_dirs_only_path both always use the default cache, which could be a caveat for adding parallelism (which is a concern and even a GSoC proposal). This patch implements threaded_check_leading_path and threading threaded_has_dirs_only_path and then implements the nonthreaded functions in terms of their threaded equivalents. No functional should be changed.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)
Show changes to 2 files +28 −2

cache.h, symlinks.c

diff --git a/cache.h b/cache.h
index e12b15f..e5e1aa4 100644
--- a/cache.h
+++ b/cache.h
@@ -950,7 +950,9 @@ struct cache_def {
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
 extern int check_leading_path(const char *name, int len);
+extern int threaded_check_leading_path(struct cache_def *cache, const char *name, int len);
 extern int has_dirs_only_path(const char *name, int len, int prefix_len);
+extern int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len);
 extern void schedule_dir_for_removal(const char *name, int len);
 extern void remove_scheduled_dirs(void);
 
diff --git a/symlinks.c b/symlinks.c
index 034943b..2900367 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -219,7 +219,20 @@ int has_symlink_leading_path(const char *name, int len)
  */
 int check_leading_path(const char *name, int len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+    return threaded_check_leading_path(&default_cache, name, len);
+}
+
+/*
+ * Return zero if path 'name' has a leading symlink component or
+ * if some leading path component does not exists.
+ *
+ * Return -1 if leading path exists and is a directory.
+ *
+ * Return path length if leading path exists and is neither a
+ * directory nor a symlink.
+ */
+int threaded_check_leading_path(struct cache_def *cache, const char *name, int len)
+{
 	int flags;
 	int match_len = lstat_cache_matchlen(cache, name, len, &flags,
 			   FL_SYMLINK|FL_NOENT|FL_DIR, USE_ONLY_LSTAT);
@@ -240,7 +253,18 @@ int check_leading_path(const char *name, int len)
  */
 int has_dirs_only_path(const char *name, int len, int prefix_len)
 {
-	struct cache_def *cache = &default_cache;	/* FIXME */
+	return threaded_has_dirs_only_path(&default_cache, name, len, prefix_len);
+}
+
+/*
+ * Return non-zero if all path components of 'name' exists as a
+ * directory.  If prefix_len > 0, we will test with the stat()
+ * function instead of the lstat() function for a prefix length of
+ * 'prefix_len', thus we then allow for symlinks in the prefix part as
+ * long as those points to real existing directories.
+ */
+int threaded_has_dirs_only_path(struct cache_def *cache, const char *name, int len, int prefix_len)
+{
 	return lstat_cache(cache, name, len,
 			   FL_DIR|FL_FULLPATH, prefix_len) &
 		FL_DIR;

-- 
1.7.3.4
Junio C Hamano· Mar 7, 2012, 23:39 UTC · re: Jared Hance · lore

Re: [PATCH v3 2/3] Add threaded versions of functions in symlinks.c.

This is identical to what is listed as "Cooking already in 'next'" in the latest issue of "What's cooking", and I've already merged it to 'master' in preparation for 1.7.10-rc0.

Thanks.
Jared Hance· Mar 7, 2012, 22:21 UTC · re: Jared Hance · lore

[PATCH v3 3/3] Use startup_info->prefix rather than prefix.

In trace_repo_setup, prefix is passed in as startup_info->prefix. But, as indicated but a FIXME comment, trace_repo_setup has access to startup_info. The prefix parameter has therefore been eliminated.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)
Show changes to 3 files +7 −7

cache.h, git.c, trace.c

diff --git a/cache.h b/cache.h
index e5e1aa4..1113296 100644
--- a/cache.h
+++ b/cache.h
@@ -1213,7 +1213,7 @@ extern void trace_printf(const char *format, ...);
 extern void trace_vprintf(const char *key, const char *format, va_list ap);
 __attribute__((format (printf, 2, 3)))
 extern void trace_argv_printf(const char **argv, const char *format, ...);
-extern void trace_repo_setup(const char *prefix);
+extern void trace_repo_setup(void);
 extern int trace_want(const char *key);
 extern void trace_strbuf(const char *key, const struct strbuf *buf);
 
diff --git a/git.c b/git.c
index 3805616..7dcc527 100644
--- a/git.c
+++ b/git.c
@@ -296,7 +296,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 		if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
 		    startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
-			trace_repo_setup(prefix);
+			trace_repo_setup();
 	}
 	commit_pager_choice();
 
diff --git a/trace.c b/trace.c
index d953416..09a470b 100644
--- a/trace.c
+++ b/trace.c
@@ -152,8 +152,7 @@ static const char *quote_crnl(const char *path)
 	return new_path;
 }
 
-/* FIXME: move prefix to startup_info struct and get rid of this arg */
-void trace_repo_setup(const char *prefix)
+void trace_repo_setup(void)
 {
 	static const char *key = "GIT_TRACE_SETUP";
 	const char *git_work_tree;
@@ -168,13 +167,14 @@ void trace_repo_setup(const char *prefix)
 	if (!(git_work_tree = get_git_work_tree()))
 		git_work_tree = "(null)";
 
-	if (!prefix)
-		prefix = "(null)";
+	if (!startup_info->prefix)
+		startup_info->prefix = "(null)";
 
 	trace_printf_key(key, "setup: git_dir: %s\n", quote_crnl(get_git_dir()));
 	trace_printf_key(key, "setup: worktree: %s\n", quote_crnl(git_work_tree));
 	trace_printf_key(key, "setup: cwd: %s\n", quote_crnl(cwd));
-	trace_printf_key(key, "setup: prefix: %s\n", quote_crnl(prefix));
+	trace_printf_key(key, "setup: prefix: %s\n",
+			 quote_crnl(startup_info->prefix));
 }
 
 int trace_want(const char *key)

-- 
1.7.3.4
Junio C Hamano· Mar 7, 2012, 23:39 UTC · re: Jared Hance · lore

Re: [PATCH v3 3/3] Use startup_info->prefix rather than prefix.

Nothing seems to have changed in this patch since the last round.
My impression has been that we agreed with your comment
    Note: I'm not quite sure if I actually agree with the first change. It makes
    sense right now if git.c is the only caller, but in the future, it might become
    less flexible.
and decided to drop this patch at least for now.

← back to recent threads