From 6fe53908f954b5683ca08d3745be1d76dd665483 Mon Sep 17 00:00:00 2001 From: Jared Hance Date: Wed, 7 Mar 2012 17:21:25 -0500 Subject: apply: do not leak patches and fragments In the while loop inside apply_patch, patch and fragments are dynamically allocated with a calloc. However, only unused patches are actually freed and the rest are left to leak. Since a list is actively built up consisting of the used patches, they can simply be iterated and freed at the end of the function. In addition, the text in fragments were not freed, primarily because they mostly point into a patch text that is freed as a whole. But there are some cases where new piece of memory is allocated and pointed by a fragment (namely, when handling a binary patch). Introduce a free_patch bitfield to mark each fragment if its text needs to be freed, and free patches, fragments and fragment text that need to be freed when we are done with the input. Signed-off-by: Jared Hance Signed-off-by: Junio C Hamano --- builtin/apply.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index c24dc546d..427c2634d 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -152,8 +152,9 @@ struct fragment { unsigned long oldpos, oldlines; unsigned long newpos, newlines; const char *patch; + unsigned free_patch:1, + rejected:1; int size; - int rejected; int linenr; struct fragment *next; }; @@ -195,6 +196,24 @@ struct patch { struct patch *next; }; +static void free_patch(struct patch *patch) +{ + while (patch) { + struct patch *patch_next = patch->next; + struct fragment *fragment = patch->fragments; + + while (fragment) { + struct fragment *fragment_next = fragment->next; + if (fragment->patch != NULL && fragment->free_patch) + free((char *)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 @@ -1741,6 +1760,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); @@ -3686,7 +3706,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); @@ -3711,8 +3730,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; @@ -3753,6 +3771,7 @@ 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; } -- cgit v1.2.1 From a604ddef737c79f4df9a943ff316e87b7c8a1de8 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Mar 2012 15:10:01 -0700 Subject: apply: rename free_patch() to free_patch_list() As that is the only logical name for a function that walks a list and frees each element on it. Signed-off-by: Junio C Hamano --- builtin/apply.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 427c2634d..aff1d9f75 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -198,19 +198,24 @@ struct patch { static void free_patch(struct patch *patch) { - while (patch) { - struct patch *patch_next = patch->next; - struct fragment *fragment = patch->fragments; - - while (fragment) { - struct fragment *fragment_next = fragment->next; - if (fragment->patch != NULL && fragment->free_patch) - free((char *)fragment->patch); - free(fragment); - fragment = fragment_next; - } - free(patch); - patch = patch_next; + struct fragment *fragment = patch->fragments; + + while (fragment) { + struct fragment *fragment_next = fragment->next; + if (fragment->patch != NULL && fragment->free_patch) + free((char *)fragment->patch); + free(fragment); + fragment = fragment_next; + } + free(patch); +} + +static void free_patch_list(struct patch *list) +{ + while (list) { + struct patch *next = list->next; + free_patch(list); + list = next; } } @@ -3771,7 +3776,7 @@ static int apply_patch(int fd, const char *filename, int options) if (summary) summary_patch_list(list); - free_patch(list); + free_patch_list(list); strbuf_release(&buf); return 0; } -- cgit v1.2.1 From 2901bbe5be41af3161fe99dede505833f26ff2bf Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 21 Mar 2012 15:18:18 -0700 Subject: 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 --- builtin/apply.c | 65 ++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 26 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index aff1d9f75..04cd8d38b 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -207,6 +207,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); } @@ -439,7 +442,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; @@ -462,11 +465,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)); @@ -631,8 +630,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; @@ -653,10 +657,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 @@ -667,8 +671,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) { @@ -865,8 +868,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; @@ -876,7 +881,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) @@ -926,13 +932,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; } @@ -951,20 +963,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; } @@ -972,6 +987,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; } @@ -979,6 +995,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; } @@ -986,6 +1003,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; } @@ -1426,7 +1444,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 " @@ -3109,6 +3128,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; } @@ -3689,15 +3709,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); } } -- cgit v1.2.1 From 5c8774330f5cc7a5e9a4f9e016e06bea6814d8b5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 27 Mar 2012 15:14:48 -0700 Subject: apply: release memory for fn_table The fn_table is used to record the result of earlier patch application in case a hand-crafted input file contains multiple patches to the same file. Both its string key (filename) and the contents are borrowed from "struct patch" that represents the previous application in the same apply_patch() call, and they do not leak, but the table itself was not freed properly. Signed-off-by: Junio C Hamano --- builtin/apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 04cd8d38b..28668c889 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3724,7 +3724,6 @@ static int apply_patch(int fd, const char *filename, int options) struct patch *list = NULL, **listp = &list; int skipped_patch = 0; - memset(&fn_table, 0, sizeof(struct string_list)); patch_input_file = filename; read_patch_file(&buf, fd); offset = 0; @@ -3791,6 +3790,7 @@ static int apply_patch(int fd, const char *filename, int options) free_patch_list(list); strbuf_release(&buf); + string_list_clear(&fn_table, 0); return 0; } -- cgit v1.2.1 From 8192a2fafcd69fe1cad2fe84c99b03c63393c117 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 21 Mar 2012 15:21:32 -0700 Subject: 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 --- builtin/apply.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 28668c889..c65fb3f8d 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -210,6 +210,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); } -- cgit v1.2.1 From 9d16c2d51412c3ebf06f88faddde3610d6e21892 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 28 Mar 2012 23:22:22 -0700 Subject: apply: free unused fragments for submodule patch We simply discarded the fragments that we are not going to use upon seeing a patch to update the submodule commit bound at path that we have not checked out. Free these fragments, not to leak them. Signed-off-by: Junio C Hamano --- builtin/apply.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index c65fb3f8d..9491d38ab 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -196,17 +196,20 @@ struct patch { struct patch *next; }; -static void free_patch(struct patch *patch) +static void free_fragment_list(struct fragment *list) { - struct fragment *fragment = patch->fragments; - - while (fragment) { - struct fragment *fragment_next = fragment->next; - if (fragment->patch != NULL && fragment->free_patch) - free((char *)fragment->patch); - free(fragment); - fragment = fragment_next; + 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); @@ -2992,7 +2995,10 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * /* * 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 { -- cgit v1.2.1 From c2066a3edad39a0fb6e894418e6602a4f7916e82 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 11 Apr 2012 14:38:31 -0700 Subject: apply: drop unused macro CHUNKSIZE is no longer used. Signed-off-by: Junio C Hamano --- builtin/apply.c | 1 - 1 file changed, 1 deletion(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 9491d38ab..3385fdb6a 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -383,7 +383,6 @@ static void say_patch_name(FILE *output, const char *pre, fputs(post, output); } -#define CHUNKSIZE (8192) #define SLOP (16) static void read_patch_file(struct strbuf *sb, int fd) -- cgit v1.2.1 From 26693ba81ccffd46fc9e01bc3346fd74f555a31a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 11 Apr 2012 14:41:42 -0700 Subject: apply: tighten constness of line buffer These point into a single line in the patch text we read from the input, and they are not used to modify it. Signed-off-by: Junio C Hamano --- builtin/apply.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 3385fdb6a..9d0006915 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1088,7 +1088,7 @@ static const char *stop_at_slash(const char *line, int llen) * 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; @@ -1215,7 +1215,7 @@ static char *git_header_name(char *line, int llen) } /* 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; @@ -1331,7 +1331,7 @@ static int parse_range(const char *line, int len, int offset, const char *expect 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; @@ -1385,7 +1385,7 @@ static void recount_diff(char *line, int size, struct fragment *fragment) * 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; @@ -1399,7 +1399,7 @@ static int parse_fragment_header(char *line, int len, struct fragment *fragment) 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; @@ -1511,7 +1511,7 @@ static void check_whitespace(const char *line, int len, unsigned ws_rule) * 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; @@ -1607,7 +1607,7 @@ static int parse_fragment(char *line, unsigned long size, return offset; } -static int parse_single_patch(char *line, unsigned long size, struct patch *patch) +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; -- cgit v1.2.1 From 92737a2201f75460dad195474a728dcf8ed79804 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Wed, 11 Apr 2012 14:43:48 -0700 Subject: apply: document buffer ownership rules across functions In general, the private functions in this file were not very much documented; even though what each of them do is reasonably self explanatory, the ownership rules for various buffers and data structures were not very obvious. Signed-off-by: Junio C Hamano --- builtin/apply.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) (limited to 'builtin/apply.c') diff --git a/builtin/apply.c b/builtin/apply.c index 9d0006915..f27f80ed7 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -151,6 +151,10 @@ 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; @@ -332,6 +336,11 @@ static void add_line_info(struct image *img, const char *bol, size_t len, unsign 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) { @@ -1607,6 +1616,14 @@ static int parse_fragment(const char *line, unsigned long size, return offset; } +/* + * 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; @@ -1700,6 +1717,11 @@ static char *inflate_it(const void *data, unsigned long size, 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, @@ -1853,6 +1875,13 @@ static int parse_binary(char *buffer, unsigned long size, struct patch *patch) 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; @@ -2413,6 +2442,11 @@ static void remove_last_line(struct image *img) 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, @@ -2484,6 +2518,11 @@ static void update_image(struct image *img, 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) @@ -2774,6 +2813,12 @@ static int apply_binary_fragment(struct image *img, struct patch *patch) 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; @@ -2981,7 +3026,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * 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)) @@ -3139,6 +3184,10 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s 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; @@ -3726,7 +3775,7 @@ static void prefix_patches(struct patch *p) 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; -- cgit v1.2.1