From: Taylor Blau <me@ttaylorr.com>
To: git@vger.kernel.org
Cc: peff@peff.net, dstolee@microsoft.com, gitster@pobox.com,
martin.agren@gmail.com
Subject: [PATCH v2 4/5] commit-graph.c: remove path normalization, comparison
Date: Mon, 3 Feb 2020 13:18:02 -0800 [thread overview]
Message-ID: <f14e95aa7eb2fa4dce2c3e1a1d81dd022efede0b.1580764494.git.me@ttaylorr.com> (raw)
In-Reply-To: <cover.1580764494.git.me@ttaylorr.com>
As of the previous patch, all calls to 'commit-graph.c' functions which
perform path normalization (for e.g., 'get_commit_graph_filename()') are
of the form 'ctx->odb->path', which is always in normalized form.
Now that there are no callers passing non-normalized paths to these
functions, ensure that future callers are bound by the same restrictions
by making these functions take a 'struct object_directory *' instead of
a 'const char *'. To match, replace all calls with arguments of the form
'ctx->odb->path' with 'ctx->odb' To recover the path, functions that
perform path manipulation simply use 'odb->path'.
Further, avoid string comparisons with arguments of the form
'odb->path', and instead prefer raw pointer comparisons, which
accomplish the same effect, but are far less brittle.
This has a pleasant side-effect of making these functions much more
robust to paths that cannot be normalized by 'normalize_path_copy()',
i.e., because they are outside of the current working directory.
For example, prior to this patch, Valgrind reports that the following
uninitialized memory read [1]:
$ ( cd t && GIT_DIR=../.git valgrind git rev-parse HEAD^ )
because 'normalize_path_copy()' can't normalize '../.git' (since it's
relative to but above of the current working directory) [2].
By using a 'struct object_directory *' directly,
'get_commit_graph_filename()' does not need to normalize, because all
paths are relative to the current working directory since they are
always read from the '->path' of an object directory.
[1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net.
[2]: The bug here is that 'get_commit_graph_filename()' returns the
result of 'normalize_path_copy()' without checking the return
value.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/commit-graph.c | 2 +-
commit-graph.c | 47 +++++++++++++++-----------------------
commit-graph.h | 2 +-
t/helper/test-read-graph.c | 6 ++---
4 files changed, 24 insertions(+), 33 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3501d9077b..b16eba2a7a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -87,7 +87,7 @@ static int graph_verify(int argc, const char **argv)
flags |= COMMIT_GRAPH_WRITE_PROGRESS;
odb = find_odb(the_repository, opts.obj_dir);
- graph_name = get_commit_graph_filename(odb->path);
+ graph_name = get_commit_graph_filename(odb);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok && errno != ENOENT)
die_errno(_("Could not open commit-graph '%s'"), graph_name);
diff --git a/commit-graph.c b/commit-graph.c
index 3af4a721ee..49541760b5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -44,30 +44,21 @@
/* Remember to update object flag allocation in object.h */
#define REACHABLE (1u<<15)
-char *get_commit_graph_filename(const char *obj_dir)
+char *get_commit_graph_filename(struct object_directory *odb)
{
- char *filename = xstrfmt("%s/info/commit-graph", obj_dir);
- char *normalized = xmalloc(strlen(filename) + 1);
- normalize_path_copy(normalized, filename);
- free(filename);
- return normalized;
+ return xstrfmt("%s/info/commit-graph", odb->path);
}
-static char *get_split_graph_filename(const char *obj_dir,
+static char *get_split_graph_filename(struct object_directory *odb,
const char *oid_hex)
{
- char *filename = xstrfmt("%s/info/commit-graphs/graph-%s.graph",
- obj_dir,
- oid_hex);
- char *normalized = xmalloc(strlen(filename) + 1);
- normalize_path_copy(normalized, filename);
- free(filename);
- return normalized;
+ return xstrfmt("%s/info/commit-graphs/graph-%s.graph", odb->path,
+ oid_hex);
}
-static char *get_chain_filename(const char *obj_dir)
+static char *get_chain_filename(struct object_directory *odb)
{
- return xstrfmt("%s/info/commit-graphs/commit-graph-chain", obj_dir);
+ return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);
}
static uint8_t oid_version(void)
@@ -330,7 +321,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)
static struct commit_graph *load_commit_graph_v1(struct repository *r,
struct object_directory *odb)
{
- char *graph_name = get_commit_graph_filename(odb->path);
+ char *graph_name = get_commit_graph_filename(odb);
struct commit_graph *g = load_commit_graph_one(graph_name);
free(graph_name);
@@ -381,7 +372,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
struct stat st;
struct object_id *oids;
int i = 0, valid = 1, count;
- char *chain_name = get_chain_filename(odb->path);
+ char *chain_name = get_chain_filename(odb);
FILE *fp;
int stat_res;
@@ -414,7 +405,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
valid = 0;
for (odb = r->objects->odb; odb; odb = odb->next) {
- char *graph_name = get_split_graph_filename(odb->path, line.buf);
+ char *graph_name = get_split_graph_filename(odb, line.buf);
struct commit_graph *g = load_commit_graph_one(graph_name);
free(graph_name);
@@ -1375,7 +1366,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
ctx->odb->path);
ctx->graph_name = strbuf_detach(&tmp_file, NULL);
} else {
- ctx->graph_name = get_commit_graph_filename(ctx->odb->path);
+ ctx->graph_name = get_commit_graph_filename(ctx->odb);
}
if (safe_create_leading_directories(ctx->graph_name)) {
@@ -1386,7 +1377,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
}
if (ctx->split) {
- char *lock_name = get_chain_filename(ctx->odb->path);
+ char *lock_name = get_chain_filename(ctx->odb);
hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);
@@ -1474,7 +1465,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
if (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {
char *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));
- char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);
+ char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb, new_base_hash);
free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);
free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);
@@ -1510,12 +1501,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
}
}
} else {
- char *graph_name = get_commit_graph_filename(ctx->odb->path);
+ char *graph_name = get_commit_graph_filename(ctx->odb);
unlink(graph_name);
}
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));
- final_graph_name = get_split_graph_filename(ctx->odb->path,
+ final_graph_name = get_split_graph_filename(ctx->odb,
ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
@@ -1557,7 +1548,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
while (g && (g->num_commits <= size_mult * num_commits ||
(max_commits && num_commits > max_commits))) {
- if (strcmp(g->odb->path, ctx->odb->path))
+ if (g->odb != ctx->odb)
break;
num_commits += g->num_commits;
@@ -1569,10 +1560,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
ctx->new_base_graph = g;
if (ctx->num_commit_graphs_after == 2) {
- char *old_graph_name = get_commit_graph_filename(g->odb->path);
+ char *old_graph_name = get_commit_graph_filename(g->odb);
if (!strcmp(g->filename, old_graph_name) &&
- strcmp(g->odb->path, ctx->odb->path)) {
+ g->odb != ctx->odb) {
ctx->num_commit_graphs_after = 1;
ctx->new_base_graph = NULL;
}
@@ -1723,7 +1714,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
if (ctx->split_opts && ctx->split_opts->expire_time)
expire_time -= ctx->split_opts->expire_time;
if (!ctx->split) {
- char *chain_file_name = get_chain_filename(ctx->odb->path);
+ char *chain_file_name = get_chain_filename(ctx->odb);
unlink(chain_file_name);
free(chain_file_name);
ctx->num_commit_graphs_after = 0;
diff --git a/commit-graph.h b/commit-graph.h
index 2448134378..5a690723b0 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -12,7 +12,7 @@
struct commit;
-char *get_commit_graph_filename(const char *obj_dir);
+char *get_commit_graph_filename(struct object_directory *odb);
int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
/*
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
index d2884efe0a..2c2f65f06c 100644
--- a/t/helper/test-read-graph.c
+++ b/t/helper/test-read-graph.c
@@ -11,12 +11,12 @@ int cmd__read_graph(int argc, const char **argv)
int open_ok;
int fd;
struct stat st;
- const char *object_dir;
+ struct object_directory *odb;
setup_git_directory();
- object_dir = get_object_directory();
+ odb = the_repository->objects->odb;
- graph_name = get_commit_graph_filename(object_dir);
+ graph_name = get_commit_graph_filename(odb);
open_ok = open_commit_graph(graph_name, &fd, &st);
if (!open_ok)
--
2.25.0.119.gaa12b7378b
next prev parent reply other threads:[~2020-02-03 21:18 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-30 23:00 [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Taylor Blau
2020-01-30 23:00 ` [PATCH 1/6] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-01-30 23:00 ` [PATCH 5/6] commit-graph.c: remove path normalization, comparison Taylor Blau
2020-01-30 23:00 ` [PATCH 6/6] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-01-30 23:00 ` [PATCH 2/6] commit-graph.h: store object directory in 'struct commit_graph' Taylor Blau
2020-01-31 6:52 ` Martin Ågren
2020-01-31 10:20 ` Jeff King
2020-01-31 19:19 ` Martin Ågren
2020-02-03 4:36 ` Taylor Blau
2020-02-03 8:36 ` Jeff King
2020-01-31 20:49 ` Junio C Hamano
2020-02-03 3:58 ` Taylor Blau
2020-01-30 23:00 ` [PATCH 4/6] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-01-30 23:00 ` [PATCH 3/6] builtin/commit-graph.c: die() with unknown '--object-dir' Taylor Blau
2020-01-31 10:30 ` [PATCH 0/6] commit-graph: use 'struct object_directory *' everywhere Jeff King
2020-01-31 13:22 ` Derrick Stolee
2020-02-03 4:38 ` Taylor Blau
2020-02-03 21:17 ` [PATCH v2 0/5] " Taylor Blau
2020-02-03 21:17 ` [PATCH v2 1/5] t5318: don't pass non-object directory to '--object-dir' Taylor Blau
2020-02-03 21:17 ` [PATCH v2 2/5] commit-graph.h: store an odb in 'struct write_commit_graph_context' Taylor Blau
2020-02-04 5:51 ` Taylor Blau
2020-02-04 19:54 ` Junio C Hamano
2020-02-04 21:28 ` Taylor Blau
2020-02-04 21:44 ` Junio C Hamano
2020-02-03 21:18 ` [PATCH v2 3/5] commit-graph.h: store object directory in 'struct commit_graph' Taylor Blau
2020-02-03 21:18 ` Taylor Blau [this message]
2020-02-03 21:18 ` [PATCH v2 5/5] commit-graph.h: use odb in 'load_commit_graph_one_fd_st' Taylor Blau
2020-02-05 12:30 ` [PATCH v2 0/5] commit-graph: use 'struct object_directory *' everywhere Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f14e95aa7eb2fa4dce2c3e1a1d81dd022efede0b.1580764494.git.me@ttaylorr.com \
--to=me@ttaylorr.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.agren@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).