Re: [PATCH 2/4] real_path: remove unsafe API

[Junio C Hamano <gitster@pobox.com>]

"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.


> 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.


>  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.

> @@ -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.

> @@ -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.

> 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.