@matheusbsbs / matheus-bagliano / commits / ad2dd5bb63

Fork 0

commit-graph.c: remove path normalization, comparison

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Feb 3, 2020 at 13:18 UTC ad2dd5bb63b6c2da0091d63463e29ae27e47a893

4 files changed +24 -33

builtin/commit-graph.c

+1 -1

@@ -88,7 +88,7 @@ static int graph_verify(int argc, const char **argv)

88 flags |= COMMIT_GRAPH_WRITE_PROGRESS;

90 odb = find_odb(the_repository, opts.obj_dir);

91 - graph_name = get_commit_graph_filename(odb->path);

91 + graph_name = get_commit_graph_filename(odb);

92 open_ok = open_commit_graph(graph_name, &fd, &st);

93 if (!open_ok && errno != ENOENT)

94 die_errno(_("Could not open commit-graph '%s'"), graph_name);

commit-graph.c

+19 -28

@@ -44,30 +44,21 @@

44 /* Remember to update object flag allocation in object.h */

45 #define REACHABLE (1u<<15)

47 -char *get_commit_graph_filename(const char *obj_dir)

47 +char *get_commit_graph_filename(struct object_directory *odb)

48 {

49 - char *filename = xstrfmt("%s/info/commit-graph", obj_dir);

50 - char *normalized = xmalloc(strlen(filename) + 1);

51 - normalize_path_copy(normalized, filename);

52 - free(filename);

53 - return normalized;

49 + return xstrfmt("%s/info/commit-graph", odb->path);

50 }

56 -static char *get_split_graph_filename(const char *obj_dir,

52 +static char *get_split_graph_filename(struct object_directory *odb,

53 const char *oid_hex)

54 {

59 - char *filename = xstrfmt("%s/info/commit-graphs/graph-%s.graph",

60 - obj_dir,

61 - oid_hex);

62 - char *normalized = xmalloc(strlen(filename) + 1);

63 - normalize_path_copy(normalized, filename);

64 - free(filename);

65 - return normalized;

55 + return xstrfmt("%s/info/commit-graphs/graph-%s.graph", odb->path,

56 + oid_hex);

57 }

68 -static char *get_chain_filename(const char *obj_dir)

59 +static char *get_chain_filename(struct object_directory *odb)

60 {

70 - return xstrfmt("%s/info/commit-graphs/commit-graph-chain", obj_dir);

61 + return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);

62 }

64 static uint8_t oid_version(void)

@@ -330,7 +321,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)

321 static struct commit_graph *load_commit_graph_v1(struct repository *r,

322 struct object_directory *odb)

323 {

333 - char *graph_name = get_commit_graph_filename(odb->path);

324 + char *graph_name = get_commit_graph_filename(odb);

325 struct commit_graph *g = load_commit_graph_one(graph_name);

326 free(graph_name);

327

@@ -381,7 +372,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,

372 struct stat st;

373 struct object_id *oids;

374 int i = 0, valid = 1, count;

384 - char *chain_name = get_chain_filename(odb->path);

375 + char *chain_name = get_chain_filename(odb);

376 FILE *fp;

377 int stat_res;

378

@@ -414,7 +405,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,

405

406 valid = 0;

407 for (odb = r->objects->odb; odb; odb = odb->next) {

417 - char *graph_name = get_split_graph_filename(odb->path, line.buf);

408 + char *graph_name = get_split_graph_filename(odb, line.buf);

409 struct commit_graph *g = load_commit_graph_one(graph_name);

410

411 free(graph_name);

@@ -1375,7 +1366,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)

1366 ctx->odb->path);

1367 ctx->graph_name = strbuf_detach(&tmp_file, NULL);

1368 } else {

1378 - ctx->graph_name = get_commit_graph_filename(ctx->odb->path);

1369 + ctx->graph_name = get_commit_graph_filename(ctx->odb);

1370 }

1371

1372 if (safe_create_leading_directories(ctx->graph_name)) {

@@ -1386,7 +1377,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)

1377 }

1378

1379 if (ctx->split) {

1389 - char *lock_name = get_chain_filename(ctx->odb->path);

1380 + char *lock_name = get_chain_filename(ctx->odb);

1381

1382 hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);

1383

@@ -1474,7 +1465,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)

1465

1466 if (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {

1467 char *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));

1477 - char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);

1468 + char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb, new_base_hash);

1469

1470 free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);

1471 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)

1501 }

1502 }

1503 } else {

1513 - char *graph_name = get_commit_graph_filename(ctx->odb->path);

1504 + char *graph_name = get_commit_graph_filename(ctx->odb);

1505 unlink(graph_name);

1506 }

1507

1508 ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));

1518 - final_graph_name = get_split_graph_filename(ctx->odb->path,

1509 + final_graph_name = get_split_graph_filename(ctx->odb,

1510 ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);

1511 ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;

1512

@@ -1557,7 +1548,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)

1548

1549 while (g && (g->num_commits <= size_mult * num_commits ||

1550 (max_commits && num_commits > max_commits))) {

1560 - if (strcmp(g->odb->path, ctx->odb->path))

1551 + if (g->odb != ctx->odb)

1552 break;

1553

1554 num_commits += g->num_commits;

@@ -1569,10 +1560,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)

1560 ctx->new_base_graph = g;

1561

1562 if (ctx->num_commit_graphs_after == 2) {

1572 - char *old_graph_name = get_commit_graph_filename(g->odb->path);

1563 + char *old_graph_name = get_commit_graph_filename(g->odb);

1564

1565 if (!strcmp(g->filename, old_graph_name) &&

1575 - strcmp(g->odb->path, ctx->odb->path)) {

1566 + g->odb != ctx->odb) {

1567 ctx->num_commit_graphs_after = 1;

1568 ctx->new_base_graph = NULL;

1569 }

@@ -1723,7 +1714,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)

1714 if (ctx->split_opts && ctx->split_opts->expire_time)

1715 expire_time -= ctx->split_opts->expire_time;

1716 if (!ctx->split) {

1726 - char *chain_file_name = get_chain_filename(ctx->odb->path);

1717 + char *chain_file_name = get_chain_filename(ctx->odb);

1718 unlink(chain_file_name);

1719 free(chain_file_name);

1720 ctx->num_commit_graphs_after = 0;

commit-graph.h

+1 -1

@@ -12,7 +12,7 @@

13 struct commit;

15 -char *get_commit_graph_filename(const char *obj_dir);

15 +char *get_commit_graph_filename(struct object_directory *odb);

16 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);

18 /*

t/helper/test-read-graph.c

+3 -3

@@ -11,12 +11,12 @@ int cmd__read_graph(int argc, const char **argv)

11 int open_ok;

12 int fd;

13 struct stat st;

14 - const char *object_dir;

14 + struct object_directory *odb;

16 setup_git_directory();

17 - object_dir = get_object_directory();

17 + odb = the_repository->objects->odb;

19 - graph_name = get_commit_graph_filename(object_dir);

19 + graph_name = get_commit_graph_filename(odb);

21 open_ok = open_commit_graph(graph_name, &fd, &st);

22 if (!open_ok)