From d51428bf17e9f17071836350299e256cac2d503a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jul 2014 00:40:43 -0400 Subject: [PATCH 1/5] receive-pack: don't copy "dir" parameter We used to do this so could pass a mutable string to enter_repo. But since 1c64b48 (enter_repo: do not modify input, 2011-10-04), this is not necessary. The resulting code is simpler, and it fixes a minor leak. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c3230817d..be8c2db26 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1125,7 +1125,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) int advertise_refs = 0; int stateless_rpc = 0; int i; - char *dir = NULL; + const char *dir = NULL; struct command *commands; struct sha1_array shallow = SHA1_ARRAY_INIT; struct sha1_array ref = SHA1_ARRAY_INIT; @@ -1160,7 +1160,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) } if (dir) usage(receive_pack_usage); - dir = xstrdup(arg); + dir = arg; } if (!dir) usage(receive_pack_usage); From 28b3563241ac13733781fb0bada37f776a39f43d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jul 2014 00:41:11 -0400 Subject: [PATCH 2/5] free ref string returned by dwim_ref A call to "dwim_ref(name, len, flags, &ref)" will allocate a new string in "ref" to return the exact ref we found. We do not consistently free it in all code paths, leading to small leaks. The worst is in get_sha1_basic, which may be called many times (e.g., by "cat-file --batch"), though it is relatively unlikely, as it only triggers on a bogus reflog specification. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 1 + builtin/show-branch.c | 1 + sha1_name.c | 4 +++- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 1a6122d3a..0bce2a63d 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -150,6 +150,7 @@ static void show_rev(int type, const unsigned char *sha1, const char *name) error("refname '%s' is ambiguous", name); break; } + free(full); } else { show_with_type(type, name); } diff --git a/builtin/show-branch.c b/builtin/show-branch.c index d87317290..b29309019 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -779,6 +779,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) sprintf(nth_desc, "%s@{%d}", *av, base+i); append_ref(nth_desc, sha1, 1); } + free(ref); } else if (all_heads + all_remotes) snarf_refs(all_heads, all_remotes); diff --git a/sha1_name.c b/sha1_name.c index 2b6322fad..cc3941eb0 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -540,8 +540,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1) char *tmp = xstrndup(str + at + 2, reflog_len); at_time = approxidate_careful(tmp, &errors); free(tmp); - if (errors) + if (errors) { + free(real_ref); return -1; + } } if (read_ref_at(real_ref, at_time, nth, sha1, NULL, &co_time, &co_tz, &co_cnt)) { From def0697167d0b3fb3c9cc1a2fcbac56e540aae48 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jul 2014 00:41:30 -0400 Subject: [PATCH 3/5] transport: fix leaks in refs_from_alternate_cb The function starts by creating a copy of the static buffer returned by real_path, but forgets to free it in the error code paths. We can solve this by jumping to the cleanup code that is already there. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- transport.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/transport.c b/transport.c index 325f03e1e..e735633a6 100644 --- a/transport.c +++ b/transport.c @@ -1369,11 +1369,11 @@ static int refs_from_alternate_cb(struct alternate_object_database *e, while (other[len-1] == '/') other[--len] = '\0'; if (len < 8 || memcmp(other + len - 8, "/objects", 8)) - return 0; + goto out; /* Is this a git repository with refs? */ memcpy(other + len - 8, "/refs", 6); if (!is_directory(other)) - return 0; + goto out; other[len - 8] = '\0'; remote = remote_get(other); transport = transport_get(remote, other); @@ -1382,6 +1382,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e, extra = extra->next) cb->fn(extra, cb->data); transport_disconnect(transport); +out: free(other); return 0; } From 649409b7bccdcd6d6e5273b2b7340cea05f77736 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jul 2014 00:42:39 -0400 Subject: [PATCH 4/5] fix memory leak parsing core.commentchar When we see the core.commentchar config option, we extract the string with git_config_string, which does two things: 1. It complains via config_error_nonbool if there is no string value. 2. It makes a copy of the string. Since we immediately parse the string into its single-character value, we only care about (1). And in fact (2) is a detriment, as it means we leak the copy. Instead, let's just check the pointer value ourselves, and parse directly from the const string we already have. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index a30cb5c07..40799a1db 100644 --- a/config.c +++ b/config.c @@ -824,11 +824,11 @@ static int git_default_core_config(const char *var, const char *value) return git_config_string(&editor_program, var, value); if (!strcmp(var, "core.commentchar")) { - const char *comment; - int ret = git_config_string(&comment, var, value); - if (!ret) - comment_line_char = comment[0]; - return ret; + if (!value) + return config_error_nonbool(var); + else + comment_line_char = value[0]; + return 0; } if (!strcmp(var, "core.askpass")) From 31bb6d37f992128eca3707d4f58ec61425742e81 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 24 Jul 2014 00:43:23 -0400 Subject: [PATCH 5/5] apply: avoid possible bogus pointer When parsing "index" lines from a git-diff, we look for a space followed by the mode. If we don't have a space, then we set our pointer to the end-of-line. However, we don't double-check that our end-of-line pointer is valid (e.g., if we got a truncated diff input), which could lead to some wrap-around pointer arithmetic. In most cases this would probably get caught by our "40 < len" check later in the function, but to be on the safe side, let's just use strchrnul to treat end-of-string the same as end-of-line. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/apply.c b/builtin/apply.c index 87439fad1..5b7a3066a 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1073,7 +1073,7 @@ static int gitdiff_index(const char *line, struct patch *patch) line = ptr + 2; ptr = strchr(line, ' '); - eol = strchr(line, '\n'); + eol = strchrnul(line, '\n'); if (!ptr || eol < ptr) ptr = eol;