Re: [PATCH v4 2/2] mv: reject a destination whose leading path is missing or a symlink

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

"Lucas Zamboni Orioli via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Lucas Zamboni Orioli <lucaszam0@gmail.com>
>
> Moving a file into a destination whose leading directories are not all
> present, real directories is only diagnosed later at rename(2), and for
> a symlinked component is not diagnosed at all.

I cannot quite parse this.  Do you mean to say something like this?

    When moving a file, if any leading directory in the destination 
    path is missing or is not a real directory, the problem is detected 
    only later when rename() is called.  Furthermore, if a leading 
    directory component is a symbolic link, the issue is not detected 
    at all.

> Three cases reach rename(2) unchecked today:
>
>   - A leading directory is missing: rename(2) fails with ENOENT,
>     reported against the source (misleading), and "git mv -n" does not
>     detect it since the dry run never reaches the syscall.

OK.  With [PATCH 1/2] in place, this is an easy case for the user to
deal with.  Either the directory name was misspelled, or the user
forgot to create intermediate levels of the destination directory.

>   - A leading component is a non-directory ("git mv x a/b" with 'a' a
>     file): rename(2) fails with ENOTDIR, again only at the syscall.

True.  'x' cannot become 'a/b' as long as 'a' is a file sitting there.

>   - A leading component is a symbolic link: "git mv" follows it. Since
>     Git tracks symlinks, the destination is really occupied by a
>     tracked object, and following it is wrong regardless of the link
>     target. The move is done on disk at the resolved location while the
>     index records the literal path, leaving the index describing a
>     worktree that does not exist. A later "git add" can reconcile it,
>     but "git mv" alone has already corrupted the state.

Yeah, that is horrible.

> Detect all three in the checking phase. Reject a destination that goes
> through a symlink with has_symlink_leading_path(), which uses lstat()
> and never follows the link, so the refusal is independent of the
> target. Then lstat() the leading directory: report "destination
> directory does not exist" for ENOENT/ENOTDIR and "destination is not a
> directory" for a non-directory. Other errors fall through to rename().

> Guard the directory check with the same condition under which rename(2)
> runs, so directory moves and sparse/out-of-cone destinations are not
> flagged incorrectly.

Nice touch.

> This changes behavior: a move through a tracked symlink that previously
> "succeeded" while corrupting the index is now refused. The other two
> cases only change when the failure is diagnosed.

Nice bugfix.

> diff --git a/builtin/mv.c b/builtin/mv.c
> index 35e504484a..535599e6be 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -22,6 +22,7 @@
>  #include "string-list.h"
>  #include "parse-options.h"
>  #include "read-cache-ll.h"
> +#include "symlinks.h"
>  
>  #include "setup.h"
>  #include "strvec.h"
> @@ -443,6 +444,40 @@ dir_check:
>  			bad = _("destination directory does not exist");
>  			goto act_on_entry;
>  		}
> +		if (has_symlink_leading_path(dst, strlen(dst))) {
> +			bad = _("destination is beyond a symbolic link");
> +			goto act_on_entry;
> +		}

With a proper helper, this part of the fix is surprisingly simple.

> +		/*
> +		 * If we are going to move SRC to DST on disk, DST's leading
> +		 * directories must already exist.
> +		 */
> +		if (!(modes[i] & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
> +		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE))) {

This small piece of logic is a duplicate of the next block that
actually performs the move.  I wonder if we can have a small helper
function that takes mode and dst_mode as parameters and returns this
value?  Then this part would become:

		if (that_function(modes[i], dst_mode)) {

and the "real thing" would become

-		if (!(mode & (INDEX | SPARSE | SKIP_WORKTREE_DIR)) &&
-		    !(dst_mode & (SKIP_WORKTREE_DIR | SPARSE)) &&
+		if (that_function(mode, dst_mode) &&
		    rename(src, dst) < 0) {
			if (ignore_errors)
				continue;
			die_errno(_("renaming '%s' failed"), src);
		}

and we will never risk them drifting apart.  Naming is the tough
part, though.  I will leave it up to you and the list to come up
with a good name that fits the semantics of what that function
computes.

> +			char *dst_dir = xstrdup(dst);
> +			char *slash = strrchr(dst_dir, '/');

Are the elements of the destinations.v[] array normalized so that
they are all full final pathnames?  I mean, 'mv A B' when B is an
existing directory would succeed, remove A, and leave 'B/A' in the
resulting working tree.  If we can depend on the preprocessing code
and the element in destinations.v[] corresponding to the move is
'B/A' (and presumably the corresponding element in the sources.v[]
array would be 'A') in such a case, then stripping the final name
component and checking whether the remainder (that is, the dirname)
is a directory, as the code below does, sounds like the right
approach.

> +			if (slash) {
> +				struct stat dir_st;
> +
> +				*slash = '\0';
> +				if (lstat(dst_dir, &dir_st) < 0) {
> +					/*
> +					 * other errors fall through to rename(),
> +					 * which reports them
> +					 */
> +					if (errno == ENOENT || errno == ENOTDIR)
> +						bad = _("destination directory does not exist");
> +				} else if (!S_ISDIR(dir_st.st_mode)) {
> +					bad = _("destination is not a directory");
> +				}
> +			}
> +			free(dst_dir);

If you did this instead

			const char *slash_ = strrchr(dst, '/');
			if (stash_) {
				char *dst_dir = xstrdup(dst);
				char *slash = &dst_dir[slash_ - dst];

then you need to allocate only if you need a copy.  I do not know if
it matters, though.  What do we do to elements in destinations.v[]
that lacks a slash?

> +			if (bad)
> +				goto act_on_entry;
> +		}

Thanks.