From 82c9d6614bcd80bc2b6646f3943971fa0ec20135 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Mar 2017 10:27:00 -0400 Subject: move odb_* declarations out of git-compat-util.h These functions were originally conceived as wrapper functions similar to xmkstemp(). They were later moved by 463db9b10 (wrapper: move odb_* to environment.c, 2010-11-06). The more appropriate place for a declaration is in cache.h. While we're at it, let's add some basic docstrings. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/cache.h b/cache.h index 61fc86e..8b7b008 100644 --- a/cache.h +++ b/cache.h @@ -1564,6 +1564,18 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1, extern void pack_report(void); /* + * Create a temporary file rooted in the object database directory. + */ +extern int odb_mkstemp(char *template, size_t limit, const char *pattern); + +/* + * Create a pack .keep file in the object database's pack directory, for + * a pack with checksum "sha1". The return value is a file descriptor opened + * for writing, or -1 on error. The name of the keep file is written to "name". + */ +extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1); + +/* * mmap the index file for the specified packfile (if it is not * already mmapped). Return 0 on success. */ diff --git a/git-compat-util.h b/git-compat-util.h index ef6d560..412703b 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -803,8 +803,6 @@ extern FILE *xfopen(const char *path, const char *mode); extern FILE *xfdopen(int fd, const char *mode); extern int xmkstemp(char *template); extern int xmkstemp_mode(char *template, int mode); -extern int odb_mkstemp(char *template, size_t limit, const char *pattern); -extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1); extern char *xgetcwd(void); extern FILE *fopen_for_writing(const char *path); -- cgit v0.10.2-6-g49f6 From 1cec8c634fb76ecee862ff88066f55b63b7d5ff7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Mar 2017 10:27:06 -0400 Subject: sha1_file.c: make pack-name helper globally accessible We provide sha1_pack_name() and sha1_pack_index_name(), but the more generic form (which takes its own strbuf and an arbitrary extension) is only used to implement the other two. Let's make it available, but clean up a few things: 1. Name it odb_pack_name(), as the original sha1_get_pack_name() is long but not all that descriptive. 2. Switch the strbuf argument to the beginning, so that it matches similar path-building functions like git_path_buf(). 3. Clean up the out-dated docstring and move it to the public declaration. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/cache.h b/cache.h index 8b7b008..baba4fe 100644 --- a/cache.h +++ b/cache.h @@ -1569,6 +1569,15 @@ extern void pack_report(void); extern int odb_mkstemp(char *template, size_t limit, const char *pattern); /* + * Generate the filename to be used for a pack file with checksum "sha1" and + * extension "ext". The result is written into the strbuf "buf", overwriting + * any existing contents. A pointer to buf->buf is returned as a convenience. + * + * Example: odb_pack_name(out, sha1, "idx") => ".git/objects/pack/pack-1234..idx" + */ +extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext); + +/* * Create a pack .keep file in the object database's pack directory, for * a pack with checksum "sha1". The return value is a file descriptor opened * for writing, or -1 on error. The name of the keep file is written to "name". diff --git a/sha1_file.c b/sha1_file.c index ec957db..df98c7f 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -203,31 +203,26 @@ static const char *alt_sha1_path(struct alternate_object_database *alt, return buf->buf; } -/* - * Return the name of the pack or index file with the specified sha1 - * in its filename. *base and *name are scratch space that must be - * provided by the caller. which should be "pack" or "idx". - */ -static char *sha1_get_pack_name(const unsigned char *sha1, - struct strbuf *buf, - const char *which) + char *odb_pack_name(struct strbuf *buf, + const unsigned char *sha1, + const char *ext) { strbuf_reset(buf); strbuf_addf(buf, "%s/pack/pack-%s.%s", get_object_directory(), - sha1_to_hex(sha1), which); + sha1_to_hex(sha1), ext); return buf->buf; } char *sha1_pack_name(const unsigned char *sha1) { static struct strbuf buf = STRBUF_INIT; - return sha1_get_pack_name(sha1, &buf, "pack"); + return odb_pack_name(&buf, sha1, "pack"); } char *sha1_pack_index_name(const unsigned char *sha1) { static struct strbuf buf = STRBUF_INIT; - return sha1_get_pack_name(sha1, &buf, "idx"); + return odb_pack_name(&buf, sha1, "idx"); } struct alternate_object_database *alt_odb_list; -- cgit v0.10.2-6-g49f6 From eaeefc3276c45ff8f8c24775b7dd93155bef7d48 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Mar 2017 10:27:12 -0400 Subject: odb_pack_keep(): stop generating keepfile name The odb_pack_keep() function generates the name of a .keep file and opens it. This has two problems: 1. It requires a fixed-size buffer to create the filename and doesn't notice when the result is truncated. 2. Of the two callers, one sometimes wants to open a filename it already has, which makes things awkward (it has to do so manually, and skips the leading-directory creation). Instead, let's have odb_pack_keep() just open the file. Generating the name isn't hard, and a future patch will switch callers over to odb_pack_name() anyway. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/index-pack.c b/builtin/index-pack.c index f4b87c6..a58bc6b 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1402,10 +1402,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name, int keep_fd, keep_msg_len = strlen(keep_msg); if (!keep_name) - keep_fd = odb_pack_keep(name, sizeof(name), sha1); - else - keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600); + snprintf(name, sizeof(name), "%s/pack/pack-%s.keep", + get_object_directory(), sha1_to_hex(sha1)); + keep_fd = odb_pack_keep(keep_name ? keep_name : name); if (keep_fd < 0) { if (errno != EEXIST) die_errno(_("cannot write keep file '%s'"), diff --git a/cache.h b/cache.h index baba4fe..0c177b7 100644 --- a/cache.h +++ b/cache.h @@ -1578,11 +1578,11 @@ extern int odb_mkstemp(char *template, size_t limit, const char *pattern); extern char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext); /* - * Create a pack .keep file in the object database's pack directory, for - * a pack with checksum "sha1". The return value is a file descriptor opened - * for writing, or -1 on error. The name of the keep file is written to "name". + * Create a pack .keep file named "name" (which should generally be the output + * of odb_pack_name). Returns a file descriptor opened for writing, or -1 on + * error. */ -extern int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1); +extern int odb_pack_keep(const char *name); /* * mmap the index file for the specified packfile (if it is not diff --git a/environment.c b/environment.c index c07fb17..4cd20d7 100644 --- a/environment.c +++ b/environment.c @@ -296,18 +296,16 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern) return xmkstemp_mode(template, mode); } -int odb_pack_keep(char *name, size_t namesz, const unsigned char *sha1) +int odb_pack_keep(const char *name) { int fd; - snprintf(name, namesz, "%s/pack/pack-%s.keep", - get_object_directory(), sha1_to_hex(sha1)); fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600); if (0 <= fd) return fd; /* slow path */ - safe_create_leading_directories(name); + safe_create_leading_directories_const(name); return open(name, O_RDWR|O_CREAT|O_EXCL, 0600); } diff --git a/fast-import.c b/fast-import.c index 64fe602..7f8371b 100644 --- a/fast-import.c +++ b/fast-import.c @@ -944,7 +944,9 @@ static char *keep_pack(const char *curr_index_name) static const char *keep_msg = "fast-import"; int keep_fd; - keep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1); + snprintf(name, sizeof(name), "%s/pack/pack-%s.keep", + get_object_directory(), sha1_to_hex(pack_data->sha1)); + keep_fd = odb_pack_keep(name); if (keep_fd < 0) die_errno("cannot create keep file"); write_or_die(keep_fd, keep_msg, strlen(keep_msg)); -- cgit v0.10.2-6-g49f6 From ba47a3088f04ac3d2833bea56ee366be1054db8d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Mar 2017 10:27:15 -0400 Subject: replace snprintf with odb_pack_name() In several places we write the name of the pack filename into a fixed-size buffer using snprintf(), but do not check the return value. As a result, a very long object directory could cause us to quietly truncate the pack filename (potentially leading to a corrupted repository, as a newly written packfile could be missing its .pack extension). We can use odb_pack_name() to do this with a strbuf (and shorten the code, as well). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/index-pack.c b/builtin/index-pack.c index a58bc6b..dcb346a 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1386,7 +1386,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, unsigned char *sha1) { const char *report = "pack"; - char name[PATH_MAX]; + struct strbuf name = STRBUF_INIT; int err; if (!from_stdin) { @@ -1402,14 +1402,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name, int keep_fd, keep_msg_len = strlen(keep_msg); if (!keep_name) - snprintf(name, sizeof(name), "%s/pack/pack-%s.keep", - get_object_directory(), sha1_to_hex(sha1)); + odb_pack_name(&name, sha1, "keep"); - keep_fd = odb_pack_keep(keep_name ? keep_name : name); + keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf); if (keep_fd < 0) { if (errno != EEXIST) die_errno(_("cannot write keep file '%s'"), - keep_name ? keep_name : name); + keep_name ? keep_name : name.buf); } else { if (keep_msg_len > 0) { write_or_die(keep_fd, keep_msg, keep_msg_len); @@ -1417,28 +1416,22 @@ static void final(const char *final_pack_name, const char *curr_pack_name, } if (close(keep_fd) != 0) die_errno(_("cannot close written keep file '%s'"), - keep_name ? keep_name : name); + keep_name ? keep_name : name.buf); report = "keep"; } } if (final_pack_name != curr_pack_name) { - if (!final_pack_name) { - snprintf(name, sizeof(name), "%s/pack/pack-%s.pack", - get_object_directory(), sha1_to_hex(sha1)); - final_pack_name = name; - } + if (!final_pack_name) + final_pack_name = odb_pack_name(&name, sha1, "pack"); if (finalize_object_file(curr_pack_name, final_pack_name)) die(_("cannot store pack file")); } else if (from_stdin) chmod(final_pack_name, 0444); if (final_index_name != curr_index_name) { - if (!final_index_name) { - snprintf(name, sizeof(name), "%s/pack/pack-%s.idx", - get_object_directory(), sha1_to_hex(sha1)); - final_index_name = name; - } + if (!final_index_name) + final_index_name = odb_pack_name(&name, sha1, "idx"); if (finalize_object_file(curr_index_name, final_index_name)) die(_("cannot store index file")); } else @@ -1464,6 +1457,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name, input_offset += err; } } + + strbuf_release(&name); } static int git_index_pack_config(const char *k, const char *v, void *cb) diff --git a/fast-import.c b/fast-import.c index 7f8371b..4d5a7f5 100644 --- a/fast-import.c +++ b/fast-import.c @@ -940,43 +940,40 @@ static const char *create_index(void) static char *keep_pack(const char *curr_index_name) { - static char name[PATH_MAX]; static const char *keep_msg = "fast-import"; + struct strbuf name = STRBUF_INIT; int keep_fd; - snprintf(name, sizeof(name), "%s/pack/pack-%s.keep", - get_object_directory(), sha1_to_hex(pack_data->sha1)); - keep_fd = odb_pack_keep(name); + odb_pack_name(&name, pack_data->sha1, "keep"); + keep_fd = odb_pack_keep(name.buf); if (keep_fd < 0) die_errno("cannot create keep file"); write_or_die(keep_fd, keep_msg, strlen(keep_msg)); if (close(keep_fd)) die_errno("failed to write keep file"); - snprintf(name, sizeof(name), "%s/pack/pack-%s.pack", - get_object_directory(), sha1_to_hex(pack_data->sha1)); - if (finalize_object_file(pack_data->pack_name, name)) + odb_pack_name(&name, pack_data->sha1, "pack"); + if (finalize_object_file(pack_data->pack_name, name.buf)) die("cannot store pack file"); - snprintf(name, sizeof(name), "%s/pack/pack-%s.idx", - get_object_directory(), sha1_to_hex(pack_data->sha1)); - if (finalize_object_file(curr_index_name, name)) + odb_pack_name(&name, pack_data->sha1, "idx"); + if (finalize_object_file(curr_index_name, name.buf)) die("cannot store index file"); free((void *)curr_index_name); - return name; + return strbuf_detach(&name, NULL); } static void unkeep_all_packs(void) { - static char name[PATH_MAX]; + struct strbuf name = STRBUF_INIT; int k; for (k = 0; k < pack_id; k++) { struct packed_git *p = all_packs[k]; - snprintf(name, sizeof(name), "%s/pack/pack-%s.keep", - get_object_directory(), sha1_to_hex(p->sha1)); - unlink_or_warn(name); + odb_pack_name(&name, p->sha1, "keep"); + unlink_or_warn(name.buf); } + strbuf_release(&name); } static int loosen_small_pack(const struct packed_git *p) @@ -1035,6 +1032,7 @@ static void end_packfile(void) die("core git rejected index %s", idx_name); all_packs[pack_id] = new_p; install_packed_git(new_p); + free(idx_name); /* Print the boundary */ if (pack_edges) { -- cgit v0.10.2-6-g49f6 From f20754802a280c57a1e5886605b6805bbf040c63 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 16 Mar 2017 10:27:20 -0400 Subject: index-pack: make pointer-alias fallbacks safer The final() function accepts a NULL value for certain parameters, and falls back to writing into a reusable "name" buffer, and then either: 1. For "keep_name", requiring all uses to do "keep_name ? keep_name : name.buf". This is awkward, and it's easy to accidentally look at the maybe-NULL keep_name. 2. For "final_index_name" and "final_pack_name", aliasing those pointers to the "name" buffer. This is easier to use, but the aliased pointers become invalid after the buffer is reused (this isn't a bug now, but it's a potential pitfall). One way to make this safer would be to introduce an extra pointer to do the aliasing, and have its lifetime match the validity of the "name" buffer. But it's still easy to accidentally use the wrong name (i.e., to use "final_pack_name" instead of the aliased pointer). Instead, let's use three separate buffers that will remain valid through the function. That makes it safe to alias the pointers and use them consistently. The extra allocations shouldn't matter, as this function is not performance sensitive. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/builtin/index-pack.c b/builtin/index-pack.c index dcb346a..88d205f 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1386,7 +1386,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name, unsigned char *sha1) { const char *report = "pack"; - struct strbuf name = STRBUF_INIT; + struct strbuf pack_name = STRBUF_INIT; + struct strbuf index_name = STRBUF_INIT; + struct strbuf keep_name_buf = STRBUF_INIT; int err; if (!from_stdin) { @@ -1402,13 +1404,13 @@ static void final(const char *final_pack_name, const char *curr_pack_name, int keep_fd, keep_msg_len = strlen(keep_msg); if (!keep_name) - odb_pack_name(&name, sha1, "keep"); + keep_name = odb_pack_name(&keep_name_buf, sha1, "keep"); - keep_fd = odb_pack_keep(keep_name ? keep_name : name.buf); + keep_fd = odb_pack_keep(keep_name); if (keep_fd < 0) { if (errno != EEXIST) die_errno(_("cannot write keep file '%s'"), - keep_name ? keep_name : name.buf); + keep_name); } else { if (keep_msg_len > 0) { write_or_die(keep_fd, keep_msg, keep_msg_len); @@ -1416,14 +1418,14 @@ static void final(const char *final_pack_name, const char *curr_pack_name, } if (close(keep_fd) != 0) die_errno(_("cannot close written keep file '%s'"), - keep_name ? keep_name : name.buf); + keep_name); report = "keep"; } } if (final_pack_name != curr_pack_name) { if (!final_pack_name) - final_pack_name = odb_pack_name(&name, sha1, "pack"); + final_pack_name = odb_pack_name(&pack_name, sha1, "pack"); if (finalize_object_file(curr_pack_name, final_pack_name)) die(_("cannot store pack file")); } else if (from_stdin) @@ -1431,7 +1433,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name, if (final_index_name != curr_index_name) { if (!final_index_name) - final_index_name = odb_pack_name(&name, sha1, "idx"); + final_index_name = odb_pack_name(&index_name, sha1, "idx"); if (finalize_object_file(curr_index_name, final_index_name)) die(_("cannot store index file")); } else @@ -1458,7 +1460,9 @@ static void final(const char *final_pack_name, const char *curr_pack_name, } } - strbuf_release(&name); + strbuf_release(&index_name); + strbuf_release(&pack_name); + strbuf_release(&keep_name_buf); } static int git_index_pack_config(const char *k, const char *v, void *cb) -- cgit v0.10.2-6-g49f6