Fix documented fixme's throughout
v3 cover Fix documented fixme's throughout
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(-)
Signature · 6 lines
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
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.
--- builtin/apply.c | 31 ++++++++++++++++++++++++++++--- 1 files changed, 28 insertions(+), 3 deletions(-)
builtin/apply.c +28 -3
@@ -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 ; }
Signature · 1 line
1.7.3.4
Re: [PATCH v3 1/3] Fix memory leak in apply_patch in apply.c.
Quoted 6 lines · 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.
...
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.
PATCH 1/2 apply: free patch->{def,old,new}_name fields
In reply to Jared HanceThese 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().
--- builtin/apply.c | 65 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 26 deletions(-)
builtin/apply.c +39 -26
@@ -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 patcif (!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); } }
Signature · 1 line
1.7.10.rc1.76.g1a8310
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.
---
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(+)
builtin/apply.c +1 -0
@@ -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; }
Signature · 1 line
1.7.10.rc1.76.g1a8310
PATCH v3 2/3 Add threaded versions of functions in symlinks.c.
In reply to Jared Hancecheck_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.
--- cache.h | 2 ++ symlinks.c | 28 ++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-)
cache.h +2 -0
@@ -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
@@ -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
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.
PATCH v3 3/3 Use startup_info->prefix rather than prefix.
In reply to Jared HanceIn 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.
--- cache.h | 2 +- git.c | 2 +- trace.c | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-)
cache.h +1 -1
@@ -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);
git.c +1 -1
@@ -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
@@ -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
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.