Fix some documented fixmes

PATCH In review Jared Hance 5 participants 18 messages Jun 15, 2016

Share & tools

Jared Hance

author

2016-06-15 22:53 UTC #0 lore raw

cover Fix some documented fixmes

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):
Use startup_info->prefix rather than prefix. Fix memory leak in apply_patch in apply.c. Add threaded versions of functions in symlinks.c.

 builtin/apply.c |   10 ++++++++--
 cache.h         |    4 +++-
 git.c           |    2 +-
 symlinks.c      |   28 ++++++++++++++++++++++++++--
 trace.c         |   10 +++++-----
 5 files changed, 43 insertions(+), 11 deletions(-)

Signature · 1 line

1.7.3.4

Jared Hance

author

2016-06-15 22:53 UTC #1 lore raw

PATCH 1/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 therefor been eliminated.

Signed-off-by Jared Hance <[email protected]>

---
 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

cache.h +1 -1

index e12b15f..baa8852 100644
@@ -1211,7 +1211,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);

git.c +1 -1

index 3805616..7dcc527 100644
@@ -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();

trace.c +5 -5

index d953416..ebacf24 100644
@@ -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)

Signature · 1 line

1.7.3.4

Junio C Hamano

2016-06-15 22:53 UTC #2 lore raw

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

Quoted 6 lines · Jared Hance

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 therefor been eliminated.
Signed-off-by Jared Hance <[email protected]>
---

This comes from a9ca8a8 (builtins: print setup info if repo is found, 2010-11-26) and hasn't ever changed over time, even across f07d6a1 (setup:
save prefix (original cwd relative to toplevel) in startup_info, 2010-12-01) that did add the necessary "prefix" field to the startup_info and was done reasonably close to the patch that wanted to have the field in the first place.

The fix looks too easy to be correct X-<; in other words, I find it hard to believe that such a triviality was left without a good reason, but I do not think of any.

Well, but perhaps something too good to be true is indeed true sometimes.

Quoted 60 lines

 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

cache.h +1 -1

index e12b15f..baa8852 100644
@@ -1211,7 +1211,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);

git.c +1 -1

index 3805616..7dcc527 100644
@@ -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();

trace.c index d953416..ebacf24 100644 --- renamed from trace.c +0 -0

a/trace.c +++ b/trace.c @@ -152,8 +152,7 @@ static const char *quote_crnl(const char *path) return new_path;
}

trace.c index d953416..ebacf24 100644 --- renamed from trace.c +5 -5

-/* 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)

Nguyen Thai Ngoc Duy

2016-06-15 22:53 UTC #3 lore raw

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

2012/3/3 Junio C Hamano <[email protected]>:

Quoted 16 lines

Quoted 6 lines · Jared Hance
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 therefor been eliminated.
Signed-off-by Jared Hance <[email protected]>
---
This comes from a9ca8a8 (builtins: print setup info if repo is found, 2010-11-26) and hasn't ever changed over time, even across f07d6a1 (setup:
save prefix (original cwd relative to toplevel) in startup_info, 2010-12-01) that did add the necessary "prefix" field to the startup_info and was done reasonably close to the patch that wanted to have the field in the first place.

Quoted 5 lines

The fix looks too easy to be correct X-<; in other words, I find it hard to believe that such a triviality was left without a good reason, but I do not think of any.
Well, but perhaps something too good to be true is indeed true sometimes.

This patch makes this function only usable when startup_info pointer is initialized. As "git" binary is the only caller, the change is ok. If non-builtin commands want to use this function, they need to initialize startup_info first.

Signature · 1 line

Duy

Junio C Hamano

2016-06-15 22:53 UTC #4 lore raw

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

Quoted 4 lines · Nguyen Thai Ngoc Duy

This patch makes this function only usable when startup_info pointer is initialized. As "git" binary is the only caller, the change is ok. If non-builtin commands want to use this function, they need to initialize startup_info first.

Hrm, that explanation is understandable, but it strongly makes me suspect that this change is making the code _more_ error prone in the longer term.

When somebody wants to add a new caller to a non-builtin, they need to think about what prefix to pass, and would realize that they need to call setup_git_directory() to get it. With the updated code, they can totally forget and call it without any initialized startup_info.

Adding a totally new command is rare, new non-builtin is rarer, and adding trace to it is even more so, so it may not be worth worrying about, but I wonder if there is a cheap way to check such a programming mistake.

Jared Hance

author

2016-06-15 22:53 UTC #5 lore raw

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

Quoted 12 lines · Junio C Hamano <gitster <at> pobox.com> writes:

Hrm, that explanation is understandable, but it strongly makes me suspect that this change is making the code _more_ error prone in the longer term.
When somebody wants to add a new caller to a non-builtin, they need to think about what prefix to pass, and would realize that they need to call setup_git_directory() to get it. With the updated code, they can totally forget and call it without any initialized startup_info.
Adding a totally new command is rare, new non-builtin is rarer, and adding trace to it is even more so, so it may not be worth worrying about, but I wonder if there is a cheap way to check such a programming mistake.

Well, there are really four options:
- Old code, where user must pass in prefix
- New code, where user must call setup_git_directory()
- Have the function deliver a nice diagnostic error message if startup_info hasn't been initialized
- Have the function call setup_git_directory() if startup_info hasn't been initialized

I think one of the latter two is the most sane here.

Jared Hance

author

2016-06-15 22:53 UTC #6 lore raw

PATCH 2/3 Fix memory leak in apply_patch in apply.c.

In reply to Jared Hance

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.

Signed-off-by Jared Hance <[email protected]>

---
 builtin/apply.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

builtin/apply.c +8 -2

index 389898f..92ebd57 100644
@@ -3685,9 +3685,9 @@ static int apply_patch(int fd, const char *filename, int options)
 	size_t offset;
 	struct strbuf buf = STRBUF_INIT;
 	struct patch *list = NULL, **listp = &list;
+	struct patch *patch_iter;
 	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,7 +3712,6 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
 			free(patch);
 			skipped_patch++;
 		}

@@ -3753,6 +3752,13 @@ static int apply_patch(int fd, const char *filename, int options)
 
 	if (summary)
 		summary_patch_list(list);
+	
+	patch_iter = list;
+	while(patch_iter != NULL) {
+	    struct patch *patch_iter_next = patch_iter->next;
+	    free(patch_iter);
+	    patch_iter = patch_iter_next;
+	}
 
 	strbuf_release(&buf);
 	return 0;

Signature · 1 line

1.7.3.4

Junio C Hamano

2016-06-15 22:53 UTC #7 lore raw

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

builtin/apply.c static int apply_patch(int fd, const char *filename, int options) · line 3715

Quoted 6 lines · Jared Hance

builtin/apply.c +0 -1

@@ -3712,7 +3712,6 @@ static int apply_patch(int fd, const char *filename, int options)
 			listp = &patch->next;
 		}
 		else {
-			/* perhaps free it a bit better? */
 			free(patch);

This "free it better" comment is not about how to free the "struct patch" itself, but is about the piece of memory pointed at it, "struct fragment", and pieces of patch text pointed at them. The patch text pointed with frag->patch starts out as a location in buf.buf (which will be freed later in this function), but IIRC there were places deeper in the callchain that replace the pointer with allocated memory.

builtin/apply.c static int apply_patch(int fd, const char *filename, int options) · line 3757

Quoted 9 lines

builtin/apply.c +3 -0

 			skipped_patch++;
 		}

@@ -3753,6 +3752,13 @@ static int apply_patch(int fd, const char *filename, int options)
 
 	if (summary)
 		summary_patch_list(list);
+	
+	patch_iter = list;
+	while(patch_iter != NULL) {

	while (patch_iter) {

Quoted 7 lines

builtin/apply.c +4 -0

+	    struct patch *patch_iter_next = patch_iter->next;
+	    free(patch_iter);
+	    patch_iter = patch_iter_next;
+	}
 
 	strbuf_release(&buf);
 	return 0;

Jared Hance

author

2016-06-15 22:53 UTC #8 lore raw

PATCH v2 1/3 Use startup_info->prefix rather than prefix.

In reply to Jared Hance

Signed-off-by Jared Hance <[email protected]>

---
 cache.h |    2 +-
 git.c   |    2 +-
 trace.c |   10 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

cache.h +1 -1

index e12b15f..baa8852 100644
@@ -1211,7 +1211,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);

git.c +1 -1

index 3805616..7dcc527 100644
@@ -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();

trace.c +5 -5

index d953416..09a470b 100644
@@ -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)

Signature · 1 line

1.7.3.4

Jeff Epler

2016-06-15 22:53 UTC #9 lore raw

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

Quoted 3 lines · Jared Hance

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 therefor been eliminated.

                                         ^^^^^^^^

Trivial typo, should be "therefore".

Jeff

Jared Hance

author

2016-06-15 22:53 UTC #10 lore raw

PATCH 3/3 Add threaded versions of functions in symlinks.c.

In reply to Jared Hance

check_leading_patch 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 <[email protected]>

---
 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

cache.h +2 -0

index baa8852..1113296 100644
@@ -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);

symlinks.c +26 -2

index 034943b..2900367 100644
@@ -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;

Signature · 1 line

1.7.3.4

Junio C Hamano

2016-06-15 22:53 UTC #11 lore raw

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

Quoted 1 line · Jared Hance

check_leading_patch and has_dirs_only_path both always use the default

s/patch/path/;

Quoted 8 lines

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 <[email protected]>
---

Ok, so these FIXME's started at 867f72b (Prepare symlink caching for thread-safety, 2009-07-09) and some of them were taken care of at b9fd284 (Export thread-safe version of 'has_symlink_leading_path()', 2009-07-09), that follows it. The latter left these two FIXMEs behind.

I wonder why. I suspect that it is merely because it wasn't necessary to call these two functions from threaded context at all.

The patch changes no behaviour and should be safe, I would think.

Quoted 64 lines

 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

cache.h +2 -0

index baa8852..1113296 100644
@@ -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);

symlinks.c +26 -2

index 034943b..2900367 100644
@@ -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;

Jared Hance

author

2016-06-15 22:53 UTC #12 lore raw

v2 cover Fix a few documents fixmes

In reply to Jared Hance

Jared Hance (3):
Use startup_info->prefix rather than prefix. Fix memory leak in apply_patch in apply.c. Add threaded versions of functions in symlinks.c.

 builtin/apply.c |   30 +++++++++++++++++++++++++++---
 cache.h         |    4 +++-
 git.c           |    2 +-
 symlinks.c      |   28 ++++++++++++++++++++++++++--
 trace.c         |   10 +++++-----
 5 files changed, 62 insertions(+), 12 deletions(-)

Signature · 13 lines

Changelog since last series submission:
    - Whitespace fixes that I should have checked for before
    - Typo fix
    - Completely redo my fix to the memory leak to address issues pointed
      out by Junio.

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. It might be wise to actually make startup_info static to git.c
for protection in the future. They should be relatively independent, though,
so the 2nd and 3rd which are probably less controversal can easily be applied.

1.7.3.4

Jared Hance

author

2016-06-15 22:53 UTC #13 lore raw

PATCH v2 2/3 Fix memory leak in apply_patch in apply.c.

In reply to Jared Hance

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 <[email protected]>

---
 builtin/apply.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

builtin/apply.c +27 -3

index 389898f..a73d339 100644
@@ -196,6 +196,30 @@ 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) {
+		if(patch->is_binary) {
+		    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

@@ -3687,7 +3711,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 +3735,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 +3776,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;
 }

Signature · 1 line

1.7.3.4

Jared Hance

author

2016-06-15 22:53 UTC #14 lore raw

Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

I just realized I'll probably need to resend this one due to stylistic errors (not using space with loops and if...).

Hopefully if there are no other mistakes I can just send in a purely cosmetically changed version to Junio.

Junio C Hamano

2016-06-15 22:53 UTC #15 lore raw

Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

In reply to Jared Hance

Quoted 5 lines · Jared Hance

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.

Right encapsulation and abstraction. Good.

Quoted 5 lines

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.

Can't we do better than "is this for a binary patch"? I find this part not very forward-thinking implementation that relies on an implementation detail that happens to hold true for today's code.

At least

        if ((buf.buf <= fragment->patch &&
            (fragment->patch < buf.buf + buf.len))
		; /* this is inside the original buffer */
	else
		free(fragment->patch);

or something?

Strictly speaking, even the above is not forward-thinking enough, as the code deep in the callchain is free to replace ->patch with an unfreeable string. I think the right way to handle this is to add a single bitfield "should_free" to the struct fragment and default it to 'false', and make the place that replace the patch field with different string responsible for flipping the bit to 'true'. Your "free_patch()" can then rely on that bit to make the decision.

Quoted 10 lines

Signed-off-by Jared Hance <[email protected]>
---
 builtin/apply.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)
builtin/apply.c +0 -0
index 389898f..a73d339 100644

builtin/apply.c struct patch { · line 200

Quoted 5 lines

builtin/apply.c +2 -0

@@ -196,6 +196,30 @@ struct patch {
 	struct patch *next;
 };
 
+static void free_patch(struct patch *patch) {
+    while(patch != NULL) {

Style:

	static void free_patch(struct patch *patch)
	{
		while (patch) {
			...

Thanks.

Jared Hance

author

2016-06-15 22:53 UTC #16 lore raw

PATCH v2 3/3 Add threaded versions of functions in symlinks.c.

In reply to Jared Hance

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 <[email protected]>

---
 cache.h    |    2 ++
 symlinks.c |   28 ++++++++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

cache.h +2 -0

index baa8852..1113296 100644
@@ -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);

symlinks.c +26 -2

index 034943b..2900367 100644
@@ -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;

Signature · 1 line

1.7.3.4

Thomas Rast

2016-06-15 22:53 UTC #17 lore raw

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

symlinks.c int has_symlink_leading_path(const char *name, int len) · line 223

Quoted 5 lines · Jared Hance

int check_leading_path(const char *name, int len) {

symlinks.c +2 -1

-	struct cache_def *cache = &default_cache;	/* FIXME */
+    return threaded_check_leading_path(&default_cache, name, len);
+}

This hunk is indent-by-space damaged.

Otherwise the patch looks good to me.

Signature · 2 lines

Thomas Rast
trast@{inf,student}.ethz.ch