@matheusbsbs / matheus-bagliano / commits / 3d7747e318

Fork 0

real_path: remove unsafe API

Returning a shared buffer invites very subtle bugs due to reentrancy or multi-threading, as demonstrated by the previous patch. There was an unfinished effort to abolish this [1]. Let's finally rid of `real_path()`, using `strbuf_realpath()` instead. This patch uses a local `strbuf` for most places where `real_path()` was previously called. However, two places return the value of `real_path()` to the caller. For them, a `static` local `strbuf` was added, effectively pushing the problem one level higher: read_gitfile_gently() get_superproject_working_tree() [1] https://lore.kernel.org/git/1480964316-99305-1-git-send-email-bmwill@google.com/ Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Alexandr Miloslavskiy committed Mar 10, 2020 at 13:11 UTC 3d7747e318532a36a263c61cdf92f2decb6424ff

13 files changed +59 -24

abspath.c

+1 -7

@@ -206,12 +206,6 @@ error_out:

206 * Resolve `path` into an absolute, cleaned-up path. The return value

207 * comes from a shared buffer.

208 */

209 -const char *real_path(const char *path)

210 -{

211 - static struct strbuf realpath = STRBUF_INIT;

212 - return strbuf_realpath(&realpath, path, 1);

213 -}

214 -

209 const char *real_path_if_valid(const char *path)

210 {

211 static struct strbuf realpath = STRBUF_INIT;

@@ -233,7 +227,7 @@ char *real_pathdup(const char *path, int die_on_error)

227

228 /*

229 * Use this to get an absolute path from a relative one. If you want

236 - * to resolve links, you should use real_path.

230 + * to resolve links, you should use strbuf_realpath.

231 */

232 const char *absolute_path(const char *path)

233 {

builtin/clone.c

+5 -1

@@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

420 struct dir_iterator *iter;

421 int iter_status;

422 unsigned int flags;

423 + struct strbuf realpath = STRBUF_INIT;

424

425 mkdir_if_missing(dest->buf, 0777);

426

@@ -454,7 +455,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

455 if (unlink(dest->buf) && errno != ENOENT)

456 die_errno(_("failed to unlink '%s'"), dest->buf);

457 if (!option_no_hardlinks) {

457 - if (!link(real_path(src->buf), dest->buf))

458 + strbuf_realpath(&realpath, src->buf, 1);

459 + if (!link(realpath.buf, dest->buf))

460 continue;

461 if (option_local > 0)

462 die_errno(_("failed to create link '%s'"), dest->buf);

@@ -468,6 +470,8 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,

470 strbuf_setlen(src, src_len);

471 die(_("failed to iterate over '%s'"), src->buf);

472 }

473 +

474 + strbuf_release(&realpath);

475 }

476

477 static void clone_local(const char *src_repo, const char *dest_repo)

builtin/commit-graph.c

+4 -1

@@ -39,14 +39,17 @@ static struct object_directory *find_odb(struct repository *r,

39 {

40 struct object_directory *odb;

41 char *obj_dir_real = real_pathdup(obj_dir, 1);

42 + struct strbuf odb_path_real = STRBUF_INIT;

44 prepare_alt_odb(r);

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

45 - if (!strcmp(obj_dir_real, real_path(odb->path)))

46 + strbuf_realpath(&odb_path_real, odb->path, 1);

47 + if (!strcmp(obj_dir_real, odb_path_real.buf))

48 break;

49 }

51 free(obj_dir_real);

52 + strbuf_release(&odb_path_real);

54 if (!odb)

55 die(_("could not find object directory matching %s"), obj_dir);

builtin/rev-parse.c

+4 -1

@@ -857,7 +857,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)

857 if (!gitdir && !prefix)

858 gitdir = ".git";

859 if (gitdir) {

860 - puts(real_path(gitdir));

860 + struct strbuf realpath = STRBUF_INIT;

861 + strbuf_realpath(&realpath, gitdir, 1);

862 + puts(realpath.buf);

863 + strbuf_release(&realpath);

864 continue;

865 }

866 }

builtin/worktree.c

+6 -3

@@ -258,7 +258,7 @@ static int add_worktree(const char *path, const char *refname,

258 const struct add_opts *opts)

259 {

260 struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;

261 - struct strbuf sb = STRBUF_INIT;

261 + struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;

262 const char *name;

263 struct child_process cp = CHILD_PROCESS_INIT;

264 struct argv_array child_env = ARGV_ARRAY_INIT;

@@ -330,9 +330,11 @@ static int add_worktree(const char *path, const char *refname,

330

331 strbuf_reset(&sb);

332 strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);

333 - write_file(sb.buf, "%s", real_path(sb_git.buf));

333 + strbuf_realpath(&realpath, sb_git.buf, 1);

334 + write_file(sb.buf, "%s", realpath.buf);

335 + strbuf_realpath(&realpath, get_git_common_dir(), 1);

336 write_file(sb_git.buf, "gitdir: %s/worktrees/%s",

335 - real_path(get_git_common_dir()), name);

337 + realpath.buf, name);

338 /*

339 * This is to keep resolve_ref() happy. We need a valid HEAD

340 * or is_git_directory() will reject the directory. Any value which

@@ -418,6 +420,7 @@ done:

420 strbuf_release(&sb_repo);

421 strbuf_release(&sb_git);

422 strbuf_release(&sb_name);

423 + strbuf_release(&realpath);

424 return ret;

425 }

426

cache.h

-1

@@ -1314,7 +1314,6 @@ static inline int is_absolute_path(const char *path)

1314 int is_directory(const char *);

1315 char *strbuf_realpath(struct strbuf *resolved, const char *path,

1316 int die_on_error);

1317 -const char *real_path(const char *path);

1317 const char *real_path_if_valid(const char *path);

1318 char *real_pathdup(const char *path, int die_on_error);

1319 const char *absolute_path(const char *path);

editor.c

+9 -2

@@ -54,7 +54,8 @@ static int launch_specified_editor(const char *editor, const char *path,

54 return error("Terminal is dumb, but EDITOR unset");

56 if (strcmp(editor, ":")) {

57 - const char *args[] = { editor, real_path(path), NULL };

57 + struct strbuf realpath = STRBUF_INIT;

58 + const char *args[] = { editor, NULL, NULL };

59 struct child_process p = CHILD_PROCESS_INIT;

60 int ret, sig;

61 int print_waiting_for_editor = advice_waiting_for_editor && isatty(2);

@@ -75,16 +76,22 @@ static int launch_specified_editor(const char *editor, const char *path,

76 fflush(stderr);

77 }

79 + strbuf_realpath(&realpath, path, 1);

80 + args[1] = realpath.buf;

81 +

82 p.argv = args;

83 p.env = env;

84 p.use_shell = 1;

85 p.trace2_child_class = "editor";

82 - if (start_command(&p) < 0)

86 + if (start_command(&p) < 0) {

87 + strbuf_release(&realpath);

88 return error("unable to start editor '%s'", editor);

89 + }

91 sigchain_push(SIGINT, SIG_IGN);

92 sigchain_push(SIGQUIT, SIG_IGN);

93 ret = finish_command(&p);

94 + strbuf_release(&realpath);

95 sig = ret - 128;

96 sigchain_pop(SIGINT);

97 sigchain_pop(SIGQUIT);

environment.c

+6 -1

@@ -254,8 +254,11 @@ static int git_work_tree_initialized;

254 */

255 void set_git_work_tree(const char *new_work_tree)

256 {

257 + struct strbuf realpath = STRBUF_INIT;

258 +

259 if (git_work_tree_initialized) {

258 - new_work_tree = real_path(new_work_tree);

260 + strbuf_realpath(&realpath, new_work_tree, 1);

261 + new_work_tree = realpath.buf;

262 if (strcmp(new_work_tree, the_repository->worktree))

263 die("internal error: work tree has already been set\n"

264 "Current worktree: %s\nNew worktree: %s",

@@ -264,6 +267,8 @@ void set_git_work_tree(const char *new_work_tree)

267 }

268 git_work_tree_initialized = 1;

269 repo_set_worktree(the_repository, new_work_tree);

270 +

271 + strbuf_release(&realpath);

272 }

273

274 const char *get_git_work_tree(void)

path.c

+1 -1

@@ -723,7 +723,7 @@ static struct passwd *getpw_str(const char *username, size_t len)

723 * then it is a newly allocated string. Returns NULL on getpw failure or

724 * if path is NULL.

725 *

726 - * If real_home is true, real_path($HOME) is used in the expansion.

726 + * If real_home is true, strbuf_realpath($HOME) is used in the expansion.

727 */

728 char *expand_user_path(const char *path, int real_home)

729 {

setup.c

+12 -3

@@ -32,6 +32,7 @@ static int abspath_part_inside_repo(char *path)

32 char *path0;

33 int off;

34 const char *work_tree = get_git_work_tree();

35 + struct strbuf realpath = STRBUF_INIT;

37 if (!work_tree)

38 return -1;

@@ -60,8 +61,10 @@ static int abspath_part_inside_repo(char *path)

61 path++;

62 if (*path == '/') {

63 *path = '\0';

63 - if (fspathcmp(real_path(path0), work_tree) == 0) {

64 + strbuf_realpath(&realpath, path0, 1);

65 + if (fspathcmp(realpath.buf, work_tree) == 0) {

66 memmove(path0, path + 1, len - (path - path0));

67 + strbuf_release(&realpath);

68 return 0;

69 }

70 *path = '/';

@@ -69,11 +72,14 @@ static int abspath_part_inside_repo(char *path)

72 }

74 /* check whole path */

72 - if (fspathcmp(real_path(path0), work_tree) == 0) {

75 + strbuf_realpath(&realpath, path0, 1);

76 + if (fspathcmp(realpath.buf, work_tree) == 0) {

77 *path0 = '\0';

78 + strbuf_release(&realpath);

79 return 0;

80 }

82 + strbuf_release(&realpath);

83 return -1;

84 }

@@ -619,6 +625,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)

625 struct stat st;

626 int fd;

627 ssize_t len;

628 + static struct strbuf realpath = STRBUF_INIT;

629

630 if (stat(path, &st)) {

631 /* NEEDSWORK: discern between ENOENT vs other errors */

@@ -669,7 +676,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)

676 error_code = READ_GITFILE_ERR_NOT_A_REPO;

677 goto cleanup_return;

678 }

672 - path = real_path(dir);

679 +

680 + strbuf_realpath(&realpath, dir, 1);

681 + path = realpath.buf;

682

683 cleanup_return:

684 if (return_error_code)

submodule.c

+3 -1

@@ -2170,6 +2170,7 @@ void absorb_git_dir_into_superproject(const char *path,

2170

2171 const char *get_superproject_working_tree(void)

2172 {

2173 + static struct strbuf realpath = STRBUF_INIT;

2174 struct child_process cp = CHILD_PROCESS_INIT;

2175 struct strbuf sb = STRBUF_INIT;

2176 const char *one_up = real_path_if_valid("../");

@@ -2231,7 +2232,8 @@ const char *get_superproject_working_tree(void)

2232 super_wt = xstrdup(cwd);

2233 super_wt[cwd_len - super_sub_len] = '\0';

2234

2234 - ret = real_path(super_wt);

2235 + strbuf_realpath(&realpath, super_wt, 1);

2236 + ret = realpath.buf;

2237 free(super_wt);

2238 }

2239 strbuf_release(&sb);

t/helper/test-path-utils.c

+4 -1

@@ -290,11 +290,14 @@ int cmd__path_utils(int argc, const char **argv)

290 }

291

292 if (argc >= 2 && !strcmp(argv[1], "real_path")) {

293 + struct strbuf realpath = STRBUF_INIT;

294 while (argc > 2) {

294 - puts(real_path(argv[2]));

295 + strbuf_realpath(&realpath, argv[2], 1);

296 + puts(realpath.buf);

297 argc--;

298 argv++;

299 }

300 + strbuf_release(&realpath);

301 return 0;

302 }

303

worktree.c

+4 -1

@@ -285,6 +285,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,

285 unsigned flags)

286 {

287 struct strbuf wt_path = STRBUF_INIT;

288 + struct strbuf realpath = STRBUF_INIT;

289 char *path = NULL;

290 int err, ret = -1;

291

@@ -336,7 +337,8 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,

337 goto done;

338 }

339

339 - ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));

340 + strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);

341 + ret = fspathcmp(path, realpath.buf);

342

343 if (ret)

344 strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),

@@ -344,6 +346,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,

346 done:

347 free(path);

348 strbuf_release(&wt_path);

349 + strbuf_release(&realpath);

350 return ret;

351 }

352