From 1cc2c772dd4dc5f9396a229e2727ca28ef25c33b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 13 Sep 2014 16:16:53 -0400 Subject: [PATCH 1/4] prune-packed: fix minor memory leak We form all of our directories in a strbuf, but never release it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/prune-packed.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c index 6879468c46..d430731d70 100644 --- a/builtin/prune-packed.c +++ b/builtin/prune-packed.c @@ -68,6 +68,7 @@ void prune_packed_objects(int opts) rmdir(pathname.buf); } stop_progress(&progress); + strbuf_release(&pathname); } int cmd_prune_packed(int argc, const char **argv, const char *prefix) From d38379ece9216735ecc0ffd76c4c4e3da217daec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 13 Sep 2014 16:19:20 -0400 Subject: [PATCH 2/4] make update-server-info more robust Since "git update-server-info" may be called automatically as part of a push or a "gc --auto", we should be robust against two processes trying to update it simultaneously. However, we currently use a fixed tempfile, which means that two simultaneous writers may step on each other's toes and end up renaming junk into place. Let's instead switch to using a unique tempfile via mkstemp. We do not want to use a lockfile here, because it's OK for two writers to simultaneously update (one will "win" the rename race, but that's OK; they should be writing the same information). While we're there, let's clean up a few other things: 1. Detect write errors. Report them and abort the update if any are found. 2. Free path memory rather than leaking it (and clean up the tempfile when necessary). 3. Use the pathdup functions consistently rather than static buffers or manually calculated lengths. This last one fixes a potential overflow of "infofile" in update_info_packs (e.g., by putting large junk into $GIT_OBJECT_DIRECTORY). However, this overflow was probably not an interesting attack vector for two reasons: a. The attacker would need to control the environment to do this, in which case it was already game-over. b. During its setup phase, git checks that the directory actually exists, which means it is probably shorter than PATH_MAX anyway. Because both update_info_refs and update_info_packs share these same failings (and largely duplicate each other), this patch factors out the improved error-checking version into a helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- server-info.c | 116 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 45 deletions(-) diff --git a/server-info.c b/server-info.c index 9ec744e9f2..d54a3d624c 100644 --- a/server-info.c +++ b/server-info.c @@ -4,45 +4,80 @@ #include "commit.h" #include "tag.h" -/* refs */ -static FILE *info_ref_fp; +/* + * Create the file "path" by writing to a temporary file and renaming + * it into place. The contents of the file come from "generate", which + * should return non-zero if it encounters an error. + */ +static int update_info_file(char *path, int (*generate)(FILE *)) +{ + char *tmp = mkpathdup("%s_XXXXXX", path); + int ret = -1; + int fd = -1; + FILE *fp = NULL; + + safe_create_leading_directories(path); + fd = mkstemp(tmp); + if (fd < 0) + goto out; + fp = fdopen(fd, "w"); + if (!fp) + goto out; + ret = generate(fp); + if (ret) + goto out; + if (fclose(fp)) + goto out; + if (adjust_shared_perm(tmp) < 0) + goto out; + if (rename(tmp, path) < 0) + goto out; + ret = 0; + +out: + if (ret) { + error("unable to update %s: %s", path, strerror(errno)); + if (fp) + fclose(fp); + else if (fd >= 0) + close(fd); + unlink(tmp); + } + free(tmp); + return ret; +} static int add_info_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data) { + FILE *fp = cb_data; struct object *o = parse_object(sha1); if (!o) return -1; - fprintf(info_ref_fp, "%s %s\n", sha1_to_hex(sha1), path); + if (fprintf(fp, "%s %s\n", sha1_to_hex(sha1), path) < 0) + return -1; + if (o->type == OBJ_TAG) { o = deref_tag(o, path, 0); if (o) - fprintf(info_ref_fp, "%s %s^{}\n", - sha1_to_hex(o->sha1), path); + if (fprintf(fp, "%s %s^{}\n", + sha1_to_hex(o->sha1), path) < 0) + return -1; } return 0; } +static int generate_info_refs(FILE *fp) +{ + return for_each_ref(add_info_ref, fp); +} + static int update_info_refs(int force) { - char *path0 = git_pathdup("info/refs"); - int len = strlen(path0); - char *path1 = xmalloc(len + 2); - - strcpy(path1, path0); - strcpy(path1 + len, "+"); - - safe_create_leading_directories(path0); - info_ref_fp = fopen(path1, "w"); - if (!info_ref_fp) - return error("unable to update %s", path1); - for_each_ref(add_info_ref, NULL); - fclose(info_ref_fp); - adjust_shared_perm(path1); - rename(path1, path0); - free(path0); - free(path1); - return 0; + char *path = git_pathdup("info/refs"); + int ret = update_info_file(path, generate_info_refs); + free(path); + return ret; } /* packs */ @@ -198,36 +233,27 @@ static void init_pack_info(const char *infofile, int force) info[i]->new_num = i; } -static void write_pack_info_file(FILE *fp) +static int write_pack_info_file(FILE *fp) { int i; - for (i = 0; i < num_pack; i++) - fprintf(fp, "P %s\n", info[i]->p->pack_name + objdirlen + 6); - fputc('\n', fp); + for (i = 0; i < num_pack; i++) { + if (fprintf(fp, "P %s\n", info[i]->p->pack_name + objdirlen + 6) < 0) + return -1; + } + if (fputc('\n', fp) == EOF) + return -1; + return 0; } static int update_info_packs(int force) { - char infofile[PATH_MAX]; - char name[PATH_MAX]; - int namelen; - FILE *fp; - - namelen = sprintf(infofile, "%s/info/packs", get_object_directory()); - strcpy(name, infofile); - strcpy(name + namelen, "+"); + char *infofile = mkpathdup("%s/info/packs", get_object_directory()); + int ret; init_pack_info(infofile, force); - - safe_create_leading_directories(name); - fp = fopen(name, "w"); - if (!fp) - return error("cannot open %s", name); - write_pack_info_file(fp); - fclose(fp); - adjust_shared_perm(name); - rename(name, infofile); - return 0; + ret = update_info_file(infofile, write_pack_info_file); + free(infofile); + return ret; } /* public */ From 3907a4078a89cffde2e2e0e61a3bd41efba96c49 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 13 Sep 2014 16:19:38 -0400 Subject: [PATCH 3/4] server-info: clean up after writing info/packs We allocate pack information in a static global list but never clean it up. This leaks memory, and means that calling update_server_info twice will generate a buggy file (it will have duplicate entries). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- server-info.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server-info.c b/server-info.c index d54a3d624c..31f4a749fb 100644 --- a/server-info.c +++ b/server-info.c @@ -233,6 +233,14 @@ static void init_pack_info(const char *infofile, int force) info[i]->new_num = i; } +static void free_pack_info(void) +{ + int i; + for (i = 0; i < num_pack; i++) + free(info[i]); + free(info); +} + static int write_pack_info_file(FILE *fp) { int i; @@ -252,6 +260,7 @@ static int update_info_packs(int force) init_pack_info(infofile, force); ret = update_info_file(infofile, write_pack_info_file); + free_pack_info(); free(infofile); return ret; } From 4489a480fdfe53e1ca78eabdab2dee00376814a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sat, 13 Sep 2014 09:28:01 +0200 Subject: [PATCH 4/4] repack: call prune_packed_objects() and update_server_info() directly Call the functions behind git prune-packed and git update-server-info directly instead of using run_command(). This is shorter, easier and quicker. Signed-off-by: Rene Scharfe Signed-off-by: Junio C Hamano --- builtin/repack.c | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index a77e743b94..6912831273 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -378,6 +378,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) /* End of pack replacement. */ if (delete_redundant) { + int opts = 0; sort_string_list(&names); for_each_string_list_item(item, &existing_packs) { char *sha1; @@ -388,25 +389,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix) if (!string_list_has_string(&names, sha1)) remove_redundant_pack(packdir, item->string); } - argv_array_push(&cmd_args, "prune-packed"); - if (quiet) - argv_array_push(&cmd_args, "--quiet"); - - memset(&cmd, 0, sizeof(cmd)); - cmd.argv = cmd_args.argv; - cmd.git_cmd = 1; - run_command(&cmd); - argv_array_clear(&cmd_args); + if (!quiet && isatty(2)) + opts |= PRUNE_PACKED_VERBOSE; + prune_packed_objects(opts); } - if (!no_update_server_info) { - argv_array_push(&cmd_args, "update-server-info"); - memset(&cmd, 0, sizeof(cmd)); - cmd.argv = cmd_args.argv; - cmd.git_cmd = 1; - run_command(&cmd); - argv_array_clear(&cmd_args); - } + if (!no_update_server_info) + update_server_info(0); remove_temporary_files(); string_list_clear(&names, 0); string_list_clear(&rollback, 0);