* [PATCH v2 1/5] move odb_* declarations out of git-compat-util.h
2017-03-16 14:26 ` [PATCH v2 0/5] minor pack-name cleanups Jeff King
@ 2017-03-16 14:27 ` Jeff King
2017-03-16 14:27 ` [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible Jeff King
` (3 subsequent siblings)
4 siblings, 0 replies; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:27 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
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 <peff@peff.net>
---
cache.h | 12 ++++++++++++
git-compat-util.h | 2 --
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index c95826971..68ad06e15 100644
--- a/cache.h
+++ b/cache.h
@@ -1634,6 +1634,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 e626851fe..8a4a3f85e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -798,8 +798,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);
--
2.12.0.623.g86ec6c963
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible
2017-03-16 14:26 ` [PATCH v2 0/5] minor pack-name cleanups Jeff King
2017-03-16 14:27 ` [PATCH v2 1/5] move odb_* declarations out of git-compat-util.h Jeff King
@ 2017-03-16 14:27 ` Jeff King
2017-03-16 14:31 ` Jeff King
2017-03-16 14:27 ` [PATCH v2 3/5] odb_pack_keep(): stop generating keepfile name Jeff King
` (2 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:27 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
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 <peff@peff.net>
---
cache.h | 9 +++++++++
sha1_file.c | 17 ++++++-----------
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index 68ad06e15..97896e2e5 100644
--- a/cache.h
+++ b/cache.h
@@ -1639,6 +1639,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
diff --git a/sha1_file.c b/sha1_file.c
index 2ee3c617a..56ef09cd3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -277,31 +277,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;
--
2.12.0.623.g86ec6c963
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible
2017-03-16 14:27 ` [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible Jeff King
@ 2017-03-16 14:31 ` Jeff King
2017-03-16 17:03 ` Ramsay Jones
0 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:31 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:
> -/*
> - * 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;
> }
Incidentally, this entire function could be implemented as:
return git_path_buf(buf, "objects/pack/pack-%s.%s",
sha1_to_hex(sha1), ext);
as the git_path() functions are smart enough to replace "objects/" with
the true object directory when necessary. I don't know if people find
that more or less readable. Since it's buried in a helper function, I
doubt it matters much either way. The git_path functions do also do some
path normalization, which might be of value.
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible
2017-03-16 14:31 ` Jeff King
@ 2017-03-16 17:03 ` Ramsay Jones
2017-03-16 17:40 ` Jeff King
0 siblings, 1 reply; 25+ messages in thread
From: Ramsay Jones @ 2017-03-16 17:03 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
On 16/03/17 14:31, Jeff King wrote:
> On Thu, Mar 16, 2017 at 10:27:06AM -0400, Jeff King wrote:
>
>> -/*
>> - * 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;
>> }
>
> Incidentally, this entire function could be implemented as:
>
> return git_path_buf(buf, "objects/pack/pack-%s.%s",
> sha1_to_hex(sha1), ext);
>
> as the git_path() functions are smart enough to replace "objects/" with
> the true object directory when necessary. I don't know if people find
> that more or less readable. Since it's buried in a helper function, I
> doubt it matters much either way. The git_path functions do also do some
> path normalization, which might be of value.
Hmm, I don't have strong feelings either way.
However, I note that the only normalization going on (that I can see)
is to remove .//* from the beginning of the resulting string. I don't
know why, but I guess it is to cater to people using the various
GIT_ environment variables doing things like:
$ GIT_OBJECT_DIRECTORY=./my-objects git ....
It has always puzzled me slightly, why the git_path functions do this
normalization, but (for example) setup_git_env(), git_path_from_env(),
get_common_dir(), ... don't! ;-)
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible
2017-03-16 17:03 ` Ramsay Jones
@ 2017-03-16 17:40 ` Jeff King
0 siblings, 0 replies; 25+ messages in thread
From: Jeff King @ 2017-03-16 17:40 UTC (permalink / raw)
To: Ramsay Jones; +Cc: git, Junio C Hamano
On Thu, Mar 16, 2017 at 05:03:03PM +0000, Ramsay Jones wrote:
> > Incidentally, this entire function could be implemented as:
> >
> > return git_path_buf(buf, "objects/pack/pack-%s.%s",
> > sha1_to_hex(sha1), ext);
> >
> > as the git_path() functions are smart enough to replace "objects/" with
> > the true object directory when necessary. I don't know if people find
> > that more or less readable. Since it's buried in a helper function, I
> > doubt it matters much either way. The git_path functions do also do some
> > path normalization, which might be of value.
>
> Hmm, I don't have strong feelings either way.
>
> However, I note that the only normalization going on (that I can see)
> is to remove .//* from the beginning of the resulting string. I don't
> know why, but I guess it is to cater to people using the various
> GIT_ environment variables doing things like:
>
> $ GIT_OBJECT_DIRECTORY=./my-objects git ....
>
> It has always puzzled me slightly, why the git_path functions do this
> normalization, but (for example) setup_git_env(), git_path_from_env(),
> get_common_dir(), ... don't! ;-)
I think there's some double-slash normalization (though not consistent
or thorough). So yeah, I'm not really sure the normalization is of great
value. I am surprised that there isn't a call to "convert_slashes()" in
there on Windows, as it seems to be sprinkled other places (like
prefix_filename()).
<shrug> If nobody's complaining, I'm happy to leave it as-is for now,
and if somebody comes up with a case where git_path normalization makes
a difference, we can look at switching it.
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 3/5] odb_pack_keep(): stop generating keepfile name
2017-03-16 14:26 ` [PATCH v2 0/5] minor pack-name cleanups Jeff King
2017-03-16 14:27 ` [PATCH v2 1/5] move odb_* declarations out of git-compat-util.h Jeff King
2017-03-16 14:27 ` [PATCH v2 2/5] sha1_file.c: make pack-name helper globally accessible Jeff King
@ 2017-03-16 14:27 ` Jeff King
2017-03-16 14:27 ` [PATCH v2 4/5] replace snprintf with odb_pack_name() Jeff King
2017-03-16 14:27 ` [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer Jeff King
4 siblings, 0 replies; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:27 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
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 <peff@peff.net>
---
builtin/index-pack.c | 6 +++---
cache.h | 8 ++++----
environment.c | 6 ++----
fast-import.c | 4 +++-
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f4b87c6c9..a58bc6bee 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 97896e2e5..cb8f48f6f 100644
--- a/cache.h
+++ b/cache.h
@@ -1649,11 +1649,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 42dc3106d..2fdba7622 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 6c13472c4..dad697653 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));
--
2.12.0.623.g86ec6c963
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v2 4/5] replace snprintf with odb_pack_name()
2017-03-16 14:26 ` [PATCH v2 0/5] minor pack-name cleanups Jeff King
` (2 preceding siblings ...)
2017-03-16 14:27 ` [PATCH v2 3/5] odb_pack_keep(): stop generating keepfile name Jeff King
@ 2017-03-16 14:27 ` Jeff King
2017-03-16 15:37 ` Ramsay Jones
2017-03-16 18:33 ` Junio C Hamano
2017-03-16 14:27 ` [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer Jeff King
4 siblings, 2 replies; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:27 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
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 <peff@peff.net>
---
builtin/index-pack.c | 27 +++++++++++----------------
fast-import.c | 28 +++++++++++++---------------
2 files changed, 24 insertions(+), 31 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a58bc6bee..dcb346ab7 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 dad697653..41a539f97 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) {
--
2.12.0.623.g86ec6c963
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()
2017-03-16 14:27 ` [PATCH v2 4/5] replace snprintf with odb_pack_name() Jeff King
@ 2017-03-16 15:37 ` Ramsay Jones
2017-03-16 18:33 ` Junio C Hamano
1 sibling, 0 replies; 25+ messages in thread
From: Ramsay Jones @ 2017-03-16 15:37 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
On 16/03/17 14:27, Jeff King wrote:
> 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 <peff@peff.net>
> ---
> builtin/index-pack.c | 27 +++++++++++----------------
> fast-import.c | 28 +++++++++++++---------------
> 2 files changed, 24 insertions(+), 31 deletions(-)
>
[snip]
> diff --git a/fast-import.c b/fast-import.c
> index dad697653..41a539f97 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);
Hmm, who deletes this, ...
> }
>
> 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);
Ah, OK. Yep, no leaks here. ;-)
>
> /* Print the boundary */
> if (pack_edges) {
>
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()
2017-03-16 14:27 ` [PATCH v2 4/5] replace snprintf with odb_pack_name() Jeff King
2017-03-16 15:37 ` Ramsay Jones
@ 2017-03-16 18:33 ` Junio C Hamano
2017-03-16 18:34 ` Jeff King
1 sibling, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2017-03-16 18:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ramsay Jones
Jeff King <peff@peff.net> writes:
> + 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";
> }
> }
The patch as-posted is obviously with less damage to the current
code, but the above makes me wonder if it makes simpler to do
if (keep_name)
strbuf_addstr(&name, keep_name);
else
odb_pack_name(&name, sha1, "keep");
so that we can always use name.buf without having to do "?:" thing.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v2 4/5] replace snprintf with odb_pack_name()
2017-03-16 18:33 ` Junio C Hamano
@ 2017-03-16 18:34 ` Jeff King
2017-03-16 18:57 ` Junio C Hamano
0 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2017-03-16 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ramsay Jones
On Thu, Mar 16, 2017 at 11:33:36AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > + 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";
> > }
> > }
>
>
> The patch as-posted is obviously with less damage to the current
> code, but the above makes me wonder if it makes simpler to do
>
> if (keep_name)
> strbuf_addstr(&name, keep_name);
> else
> odb_pack_name(&name, sha1, "keep");
>
> so that we can always use name.buf without having to do "?:" thing.
See the next patch. :)
-Peff
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer
2017-03-16 14:26 ` [PATCH v2 0/5] minor pack-name cleanups Jeff King
` (3 preceding siblings ...)
2017-03-16 14:27 ` [PATCH v2 4/5] replace snprintf with odb_pack_name() Jeff King
@ 2017-03-16 14:27 ` Jeff King
2017-03-16 15:42 ` Ramsay Jones
4 siblings, 1 reply; 25+ messages in thread
From: Jeff King @ 2017-03-16 14:27 UTC (permalink / raw)
To: git; +Cc: Ramsay Jones, Junio C Hamano
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 <peff@peff.net>
---
builtin/index-pack.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dcb346ab7..88d205f85 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)
--
2.12.0.623.g86ec6c963
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer
2017-03-16 14:27 ` [PATCH v2 5/5] index-pack: make pointer-alias fallbacks safer Jeff King
@ 2017-03-16 15:42 ` Ramsay Jones
0 siblings, 0 replies; 25+ messages in thread
From: Ramsay Jones @ 2017-03-16 15:42 UTC (permalink / raw)
To: Jeff King, git; +Cc: Junio C Hamano
On 16/03/17 14:27, Jeff King wrote:
> 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 <peff@peff.net>
> ---
> builtin/index-pack.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index dcb346ab7..88d205f85 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)
>
Yep, much better.
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 25+ messages in thread