From fda3e2cf017493ad9051c027a527e5c995ebfaf1 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 22 Mar 2016 21:58:40 +0100 Subject: [PATCH 1/4] builtin/apply: get rid of useless 'name' variable While at it put an 'else' on the same line as the previous '}'. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/apply.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 42c610e2ec..465f954d9f 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, return find_name(line, NULL, p_value, TERM_TAB); if (orig_name) { - int len; - const char *name; + int len = strlen(orig_name); char *another; - name = orig_name; - len = strlen(name); if (isnull) - die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), name, linenr); + die(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), + orig_name, linenr); another = find_name(line, NULL, p_value, TERM_TAB); - if (!another || memcmp(another, name, len + 1)) + if (!another || memcmp(another, orig_name, len + 1)) die((side == DIFF_NEW_NAME) ? _("git apply: bad git-diff - inconsistent new filename on line %d") : _("git apply: bad git-diff - inconsistent old filename on line %d"), linenr); free(another); return orig_name; - } - else { + } else { /* expect "/dev/null" */ if (memcmp("/dev/null", line, 9) || line[9] != '\n') die(_("git apply: bad git-diff - expected /dev/null on line %d"), linenr); From db354b7f1be0d6d7b8cd85800f83a1d2d90cc4a5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 22 Mar 2016 14:41:08 -0700 Subject: [PATCH 2/4] apply: remove unused call to free() in gitdiff_{old,new}name() These two functions keep a copy of filename it was given, let gitdiff_verify_name() to rewrite it to a new filename and then free the original if they receive a newly minted filename. However (1) when the original name is NULL, gitdiff_verify_name() returns either NULL or a newly minted value. Either case, we do not have to worry about calling free() on the original NULL. (2) when the original name is not NULL, gitdiff_verify_name() either returns that as-is, or calls die() when it finds inconsistency in the patch. When the function returns, we know that "if ()" statement always is false. Noticed by Christian Couder. Signed-off-by: Junio C Hamano --- builtin/apply.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 465f954d9f..4afc94fdbd 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -953,21 +953,15 @@ 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, DIFF_OLD_NAME); - 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, DIFF_NEW_NAME); - if (orig != patch->new_name) - free(orig); return 0; } From 484e77615872b2a62775a6a2dcb1cad0f529c5d3 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Fri, 18 Mar 2016 13:30:41 +0100 Subject: [PATCH 3/4] builtin/apply: handle parse_binary() failure In parse_binary() there is: forward = parse_binary_hunk(&buffer, &size, &status, &used); if (!forward && !status) /* there has to be one hunk (forward hunk) */ return error(_("unrecognized binary patch at line %d"), linenr-1); so parse_binary() can return -1, because that's what error() returns. Also parse_binary_hunk() sets "status" to -1 in case of error and parse_binary() does "if (status) return status;". In this case parse_chunk() should not add -1 to the patchsize it computes. It is better for future libification efforts to make it just return -1. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/apply.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/builtin/apply.c b/builtin/apply.c index 4afc94fdbd..dbdfa9b646 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1863,6 +1863,11 @@ static struct fragment *parse_binary_hunk(char **buf_p, return NULL; } +/* + * Returns: + * -1 in case of error, + * the length of the parsed binary patch otherwise + */ static int parse_binary(char *buffer, unsigned long size, struct patch *patch) { /* @@ -2008,6 +2013,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch) linenr++; used = parse_binary(buffer + hd + llen, size - hd - llen, patch); + if (used < 0) + return -1; if (used) patchsize = used + llen; else From 7a6a44c2dc7a59ef8e117c2f1ea3c06ce6a6ff1f Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 16 Mar 2016 20:35:11 +0100 Subject: [PATCH 4/4] builtin/apply: free patch when parse_chunk() fails When parse_chunk() fails it can return -1, for example when find_header() doesn't find a patch header. In this case it's better in apply_patch() to free the "struct patch" that we just allocated instead of leaking it. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/apply.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/builtin/apply.c b/builtin/apply.c index dbdfa9b646..ce3b77853c 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -4371,8 +4371,10 @@ static int apply_patch(int fd, const char *filename, int options) patch->inaccurate_eof = !!(options & INACCURATE_EOF); patch->recount = !!(options & RECOUNT); nr = parse_chunk(buf.buf + offset, buf.len - offset, patch); - if (nr < 0) + if (nr < 0) { + free_patch(patch); break; + } if (apply_in_reverse) reverse_patches(patch); if (use_patch(patch)) {