Re: [PATCH 2/4] real_path: remove unsafe API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 6, 2020, 22:12 UTC
- Message-ID
- <xmqq4kv12kvx.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <039d3d368662f3a7e208fa7aa47549ca2654574a.1583521396.git.gitgitgadget@gmail.com>
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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()
Yeah, I noticed that while reading the patch. It is not making it any worse, and other parts of the patch made tons of sense (except one small thing).
It was especially pleasing to see that care has been taken to avoid introducing strbuf leaks.
Show 19 quoted lines
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 1ad26f4d8c8..e5c2a229a11 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -420,6 +420,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> struct dir_iterator *iter;
> int iter_status;
> unsigned int flags;
> + struct strbuf realpath = STRBUF_INIT;
>
> mkdir_if_missing(dest->buf, 0777);
>
> @@ -454,7 +455,9 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> if (unlink(dest->buf) && errno != ENOENT)
> die_errno(_("failed to unlink '%s'"), dest->buf);
> if (!option_no_hardlinks) {
> - if (!link(real_path(src->buf), dest->buf))
> + strbuf_reset(&realpath);
> + strbuf_realpath(&realpath, src->buf, 1);This is inside a loop, so "struct strbuf realpath" here in the second or subsequent iteration may not be empty; it is true that strbuf_reset() is necessary _somewhere_ in the loop to discard the path that was created for the previous iteration.
If my reading of the code is correct, however, the first thing that is done by strbuf_realpath() is to empty the output buffer by using strbuf_reset() indirectly via get_root_part(). Calling strbuf_reset() here should not hurt, but it is unnecessary, I would think. An even worse effect such a redundant strbuf_reset() has is that by repeatedly seeing the "reset then call realpath" pattern, readers who do not read the implementation of strbuf_realpath() might mistakenly think that
strbuf_addf(&message, "the path '%s' is really ", path); strbuf_realpath(&message, path);
is how realpath() is expected to be used, i.e. keep the current contents in the buffer and append the resolved path to it.
Show 16 quoted lines
> static void clone_local(const char *src_repo, const char *dest_repo)
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4a70b33fb5f..3d7ec640e01 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -39,14 +39,18 @@ static struct object_directory *find_odb(struct repository *r,
> {
> struct object_directory *odb;
> char *obj_dir_real = real_pathdup(obj_dir, 1);
> + struct strbuf odb_path_real = STRBUF_INIT;
>
> prepare_alt_odb(r);
> for (odb = r->objects->odb; odb; odb = odb->next) {
> - if (!strcmp(obj_dir_real, real_path(odb->path)))
> + strbuf_reset(&odb_path_real);
> + strbuf_realpath(&odb_path_real, odb->path, 1);Likewise.
Show 7 quoted lines
> @@ -60,8 +61,11 @@ static int abspath_part_inside_repo(char *path)
> path++;
> if (*path == '/') {
> *path = '\0';
> - if (fspathcmp(real_path(path0), work_tree) == 0) {
> + strbuf_reset(&realpath);
> + strbuf_realpath(&realpath, path0, 1);Likewise.
Show 7 quoted lines
> @@ -69,11 +73,15 @@ static int abspath_part_inside_repo(char *path)
> }
>
> /* check whole path */
> - if (fspathcmp(real_path(path0), work_tree) == 0) {
> + strbuf_reset(&realpath);
> + strbuf_realpath(&realpath, path0, 1);Likewise.
Show 14 quoted lines
> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index 409034cf4ee..40548d31dfe 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -290,11 +290,15 @@ int cmd__path_utils(int argc, const char **argv)
> }
>
> if (argc >= 2 && !strcmp(argv[1], "real_path")) {
> + struct strbuf realpath = STRBUF_INIT;
> while (argc > 2) {
> - puts(real_path(argv[2]));
> + strbuf_reset(&realpath);
> + strbuf_realpath(&realpath, argv[2], 1);
> + puts(realpath.buf);Likewise.
Thanks.