Merge branch 'jh/apply-free-patch'
authorJunio C Hamano <gitster@pobox.com>
Mon, 23 Apr 2012 19:52:18 +0000 (12:52 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 23 Apr 2012 19:52:18 +0000 (12:52 -0700)
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

1  2 
builtin/apply.c
diff --combined builtin/apply.c
index 389898f13364eb640077c1d82fefea98d9d3755f,f27f80ed76cd17370b2477ff6223abbd7b67c7d9..799bb5e906c1e6de6c95cad4b9f4ee8fedf258f0
@@@ -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;
                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;
                        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
        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;
                        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;
  }
  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;
  }
  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;
  }
  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;
  }
  
  /* 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;
  
   * 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;
  
        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;
  
                                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;
        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,
  
        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,
        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))
                                /*
                                 * 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);
        }
  }
  
  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;
                        listp = &patch->next;
                }
                else {
-                       /* perhaps free it a bit better? */
-                       free(patch);
+                       free_patch(patch);
                        skipped_patch++;
                }
                offset += nr;
        if (summary)
                summary_patch_list(list);
  
+       free_patch_list(list);
        strbuf_release(&buf);
+       string_list_clear(&fn_table, 0);
        return 0;
  }