[PATCH] fix `git mv existing-dir non-existing-dir`*

[PATCH] fix `git mv existing-dir non-existing-dir`*

[Sebastian Thiel via GitGitGadget]

From: Sebastian Thiel <sebastian.thiel@icloud.com>

*in some environments.

When moving a directory onto another with `gix mv`
various checks are performed. One of of these
validates that the destination is not an existing
file.

When calling `lstat` on the destination path and
it fails as the path doesn't exist, some
environments seem to overwrite the passed  in
`stat` memory nonetheless.
(I observed this issue on debian 12 of x86_64,
running on OrbStack on ARM, emulated with Rosetta)

This would affect the code that followed as it
would still acccess a now
modified `st` structure, which now seems to
contain uninitialized memory.
`S_ISDIR(st_dir_mode)` would then typically
return false causing the code to run into a bad
case.

The fix avoids overwriting the existing `st`
structure, providing an alternative that exists
only for that purpose.

Note that this patch minimizes complexity instead of stack-size.

Signed-off-by: Sebastian Thiel <sebastian.thiel@icloud.com>
---
    fix git mv existing-dir non-existing-dir*
    
    fix git mv existing-dir non-existing-dir*
    
    *in some environments.
    
    When moving a directory onto another with gix mv various checks are
    performed. One of of these validates that the destination is not an
    existing file.
    
    When calling lstat on the destination path and it fails as the path
    doesn't exist, some environments seem to overwrite the passed in stat
    memory nonetheless. (I observed this issue on debian 12 of x86_64,
    running on OrbStack on ARM, emulated with Rosetta)
    
    This would affect the code that followed as it would still acccess a now
    modified st structure, which now seems to contain uninitialized memory.
    S_ISDIR(st_dir_mode) would then typically return false causing the code
    to run into a bad case.
    
    The fix avoids overwriting the existing st structure, providing an
    alternative that exists only for that purpose.
    
    ------------------------------------------------------------------------
    
    It's worth pointing out that the test demonstrates this case only if one
    happens to execute it in one of the environments that happen to have an
    lstat that writes into stat even on error. Thus it already worked for me
    on MacOS, even without the patch applied, which matches my observation
    that a certain script works there but doesn't work on the VM.
    
    Even though the patch now minimizes size, I can imagine one might
    instead want to rather copy st.st_mode to protect only the relevant
    field from being affected by potential rewrites of st later on.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1561%2FByron%2Ffix-mv-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1561/Byron/fix-mv-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1561

 builtin/mv.c  | 4 ++--
 t/t7001-mv.sh | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index fa84fcb20d8..05e7156034e 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	int src_dir_nr = 0, src_dir_alloc = 0;
 	struct strbuf a_src_dir = STRBUF_INIT;
 	enum update_mode *modes, dst_mode = 0;
-	struct stat st;
+	struct stat st, dest_st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
 	struct cache_entry *ce;
@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			goto act_on_entry;
 		}
 		if (S_ISDIR(st.st_mode)
-		    && lstat(dst, &st) == 0) {
+		    && lstat(dst, &dest_st) == 0) {
 			bad = _("cannot move directory over file");
 			goto act_on_entry;
 		}
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 898a9205328..9894bc45ee6 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -174,6 +174,12 @@ test_expect_success 'do not move directory over existing directory' '
 	test_must_fail git mv path2 path0
 '
 
+test_expect_success 'rename directory to non-existing directory' '
+	mkdir dir-a && touch dir-a/f &&
+	git add dir-a &&
+	git mv dir-a non-existing-dir
+'
+
 test_expect_success 'move into "."' '
 	git mv path1/path2/ .
 '

base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
-- 
gitgitgadget

Re: [PATCH] fix `git mv existing-dir non-existing-dir`*

[Junio C Hamano]

"Sebastian Thiel via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sebastian Thiel <sebastian.thiel@icloud.com>
>
> *in some environments.

Please do not chop a single sentence in the middle and mark that
fact with an asterisk nobody understands what it means.

    Subject: [PATCH] mv: handle lstat() failure correctly

perhaps?

> When moving a directory onto another with `gix mv` various checks
> are performed. One of of these validates that the destination is
> not an existing file.
>
> When calling `lstat` on the destination path and it fails as the
> path doesn't exist, some environments seem to overwrite the passed
> in `stat` memory nonetheless.  (I observed this issue on debian 12
> of x86_64, running on OrbStack on ARM, emulated with Rosetta)

Very cleanly written, except "gix" -> "git".

POSIX does not seem to specify what should happen to buf when the
call fails, which I take to mean that its contents can become any
garbage at that point.

> diff --git a/builtin/mv.c b/builtin/mv.c
> index fa84fcb20d8..05e7156034e 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  	int src_dir_nr = 0, src_dir_alloc = 0;
>  	struct strbuf a_src_dir = STRBUF_INIT;
>  	enum update_mode *modes, dst_mode = 0;
> -	struct stat st;
> +	struct stat st, dest_st;
>  	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
>  	struct lock_file lock_file = LOCK_INIT;
>  	struct cache_entry *ce;
> @@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>  			goto act_on_entry;
>  		}
>  		if (S_ISDIR(st.st_mode)
> -		    && lstat(dst, &st) == 0) {
> +		    && lstat(dst, &dest_st) == 0) {

This is good.  After this "if (S_ISDIR)" thing, there is another "if
(S_ISDIR)" on the same st.st_mode, so clobbering st like the
original was a stupid thing to do.

>  			bad = _("cannot move directory over file");

What is curious is that dest_st.st_mode, after lstat on dst
succeeds, is never checked, even though the error message claims
that it detected an attempt to move directory over file.  What
should happen when the user did this then?

    $ git mv existing-dir another-existing-dir

Shouldn't it do something similar to

    $ mv D1 D2

which is to move the entire hierarchy of D1 and make it appear at
D2/D1?

Even if the answer to the above question is "yes", that is a
separate bugfix, so let's not get distracted and see how our
test looks like.

>  			goto act_on_entry;
>  		}
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 898a9205328..9894bc45ee6 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -174,6 +174,12 @@ test_expect_success 'do not move directory over existing directory' '
>  	test_must_fail git mv path2 path0
>  '
>  
> +test_expect_success 'rename directory to non-existing directory' '
> +	mkdir dir-a && touch dir-a/f &&

One command per line, and reserve the use of "touch" to cases where
you care about the timestamps, not existence.  I.e.

	mkdir dir-a &&
	>dir-a/f &&

> +	git add dir-a &&
> +	git mv dir-a non-existing-dir
> +'

OK, there is no guarantee that this would fail on a system whose
lstat() may clobber buf when it notices that the path does not
exist, but it is a good test to have.

Thanks.

Re: [PATCH] fix `git mv existing-dir non-existing-dir`*

[Torsten Bögershausen]

On Tue, Aug 08, 2023 at 10:36:54AM -0700, Junio C Hamano wrote:
> "Sebastian Thiel via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Sebastian Thiel <sebastian.thiel@icloud.com>
> >

The patch makes sense to me, Junio's comments included.

> Shouldn't it do something similar to
>
>     $ mv D1 D2

Couldn't resist to test it ;-)

The result would be
 renamed: D1/file1 -> D2/D1/file1

Re: [PATCH] fix `git mv existing-dir non-existing-dir`*

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On Tue, Aug 08, 2023 at 10:36:54AM -0700, Junio C Hamano wrote:
>> "Sebastian Thiel via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Sebastian Thiel <sebastian.thiel@icloud.com>
>> >
>
> The patch makes sense to me, Junio's comments included.
>
>> Shouldn't it do something similar to
>>
>>     $ mv D1 D2
>
> Couldn't resist to test it ;-)
>
> The result would be
>  renamed: D1/file1 -> D2/D1/file1

Sure.  The lstat() in question is about the case where a different
D2/D1 already exists, either as a file (which will definitely break
as we do not and should not do unlink-and-then-mkdir) or as a
directory (which may be OK in some cases to get a union of the
contents in the original D1 and D2/D1, but in general not a good
idea).

And in the latter case, i.e. when D2/D1 exists as a directory, we
should not say "cannot move directory over file".  So, the check
that does not care what the dest_dir's type is fine. but the error
message is wrong.

    "cannot move directory over file, source=D1, destination=D1/D2"

is the message we would get in such a case.  We probably just should
say

    "destination already exists, source=D1, destination=D1/D2"

or something like that.

[PATCH] mv: fix error for moving directory to another

[Junio C Hamano]

If both directories D1 and D2 already exists, and further there is a
filesystem entity D2/D1, "git mv D1 D2" would fail, and we get an
error message that says:

    "cannot move directory over file, source=D1, destination=D2/D1"

regardless of the type of existing "D2/D1".  If it is a file, the
message is correct, but if it is a directory, it is not (we could
make the D2/D1 directory a union of its original contents and what
was in D1/, but that is not what we do).

The code that decies to issue the error message only checks for
existence of "D2/D1" and does not care what kind of thing sits at
the path.

Rephrase the message to say

    "destination already exists, source=D1, destination=D2/D1"

that would be suitable for any kind of thing being in the way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Just so that we do not forget what we discussed.  It is kind of
   interesting that no tests need adjustment for this change, which
   make me suspect how good our test coverage is.

   This patch has a trivial textual conflict with Sebastian's patch,
   but the resolution should be obvious.

 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 665bd27448..80fc7a3c70 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 		}
 		if (S_ISDIR(st.st_mode)
 		    && lstat(dst, &st) == 0) {
-			bad = _("cannot move directory over file");
+			bad = _("destination already exists");
 			goto act_on_entry;
 		}
 
-- 
2.42.0-rc1

Re: [PATCH] fix `git mv existing-dir non-existing-dir`*

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> What is curious is that dest_st.st_mode, after lstat on dst
> succeeds, is never checked, even though the error message claims
> that it detected an attempt to move directory over file.  What
> should happen when the user did this then?
>
>     $ git mv existing-dir another-existing-dir
>
> Shouldn't it do something similar to
>
>     $ mv D1 D2
>
> which is to move the entire hierarchy of D1 and make it appear at
> D2/D1?

Ah, that case is handled in a different codepath, so this lstat of
dst needs to check only the existence.  So there is no (unrelated)
bug there.  Sorry for the noise.

[PATCH v2] fix `git mv existing-dir non-existing-dir` in some environments.

[Sebastian Thiel via GitGitGadget]

From: Sebastian Thiel <sebastian.thiel@icloud.com>

When moving a directory onto another with `git mv` various checks are
performed. One of of these validates that the destination is not existing.

When calling `lstat` on the destination path and it fails as the path
doesn't exist, some environments seem to overwrite the passed  in
`stat` memory nonetheless (I observed this issue on debian 12 of x86_64,
running on OrbStack on ARM, emulated with Rosetta).

This would affect the code that followed as it would still acccess a now
modified `st` structure, which now seems to contain uninitialized memory.
`S_ISDIR(st_dir_mode)` would then typically return false causing the code
to run into a bad case.

The fix avoids overwriting the existing `st` structure, providing an
alternative that exists only for that purpose.

Note that this patch minimizes complexity instead of stack-frame size.

Signed-off-by: Sebastian Thiel <sebastian.thiel@icloud.com>
---
    [PATCH] mv: handle lstat() failure correctly
    
    When moving a directory onto another with git mv various checks are
    performed. One of of these validates that the destination is not
    existing.
    
    When calling lstat on the destination path and it fails as the path
    doesn't exist, some environments seem to overwrite the passed in stat
    memory nonetheless (I observed this issue on debian 12 of x86_64,
    running on OrbStack on ARM, emulated with Rosetta).
    
    This would affect the code that followed as it would still acccess a now
    modified st structure, which now seems to contain uninitialized memory.
    S_ISDIR(st_dir_mode) would then typically return false causing the code
    to run into a bad case.
    
    The fix avoids overwriting the existing st structure, providing an
    alternative that exists only for that purpose.
    
    
    Note that this patch minimizes complexity instead of stack-frame size.
    ======================================================================
    
    It's worth pointing out that the test demonstrates this case only if one
    happens to execute it in one of the environments that happen to have an
    lstat that writes into stat even on error. Thus it already worked for me
    on MacOS, even without the patch applied, which matches my observation
    that a certain script works there but doesn't work on the VM.
    
    Even though the patch now minimizes size, I can imagine one might
    instead want to rather copy st.st_mode to protect only the relevant
    field from being affected by potential rewrites of st later on.
    
    Changes since v1:
    
     * replaced previous title with recommendation by Junio C Hermano
     * improved formatting of commit message and renamed gix to git. Let's
       call that a typo
     * apply Junio C Hermano's suggestions to test-case
     * I refrained from changing the error message as this would mean all
       translations need adjustment (and I don't know how this is tracked
       then)
    
    I also want to apologise for the possibly terrible formatting and the
    repetition - it feels strange but is what gitgadget seems to suggests.
    Further, it's my honour to submit a patch to git and interact with the
    maintainers, it's like meeting my idols!

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1561%2FByron%2Ffix-mv-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1561/Byron/fix-mv-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1561

Range-diff vs v1:

 1:  ad0e6754e2d ! 1:  8908fd228fe fix `git mv existing-dir non-existing-dir`*
     @@ Metadata
      Author: Sebastian Thiel <sebastian.thiel@icloud.com>
      
       ## Commit message ##
     -    fix `git mv existing-dir non-existing-dir`*
     +    fix `git mv existing-dir non-existing-dir` in some environments.
      
     -    *in some environments.
     +    When moving a directory onto another with `git mv` various checks are
     +    performed. One of of these validates that the destination is not existing.
      
     -    When moving a directory onto another with `gix mv`
     -    various checks are performed. One of of these
     -    validates that the destination is not an existing
     -    file.
     +    When calling `lstat` on the destination path and it fails as the path
     +    doesn't exist, some environments seem to overwrite the passed  in
     +    `stat` memory nonetheless (I observed this issue on debian 12 of x86_64,
     +    running on OrbStack on ARM, emulated with Rosetta).
      
     -    When calling `lstat` on the destination path and
     -    it fails as the path doesn't exist, some
     -    environments seem to overwrite the passed  in
     -    `stat` memory nonetheless.
     -    (I observed this issue on debian 12 of x86_64,
     -    running on OrbStack on ARM, emulated with Rosetta)
     +    This would affect the code that followed as it would still acccess a now
     +    modified `st` structure, which now seems to contain uninitialized memory.
     +    `S_ISDIR(st_dir_mode)` would then typically return false causing the code
     +    to run into a bad case.
      
     -    This would affect the code that followed as it
     -    would still acccess a now
     -    modified `st` structure, which now seems to
     -    contain uninitialized memory.
     -    `S_ISDIR(st_dir_mode)` would then typically
     -    return false causing the code to run into a bad
     -    case.
     +    The fix avoids overwriting the existing `st` structure, providing an
     +    alternative that exists only for that purpose.
      
     -    The fix avoids overwriting the existing `st`
     -    structure, providing an alternative that exists
     -    only for that purpose.
     -
     -    Note that this patch minimizes complexity instead of stack-size.
     +    Note that this patch minimizes complexity instead of stack-frame size.
      
          Signed-off-by: Sebastian Thiel <sebastian.thiel@icloud.com>
      
     @@ t/t7001-mv.sh: test_expect_success 'do not move directory over existing director
       '
       
      +test_expect_success 'rename directory to non-existing directory' '
     -+	mkdir dir-a && touch dir-a/f &&
     ++	mkdir dir-a &&
     ++	>dir-a/f &&
      +	git add dir-a &&
      +	git mv dir-a non-existing-dir
      +'


 builtin/mv.c  | 4 ++--
 t/t7001-mv.sh | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index fa84fcb20d8..05e7156034e 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -184,7 +184,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	int src_dir_nr = 0, src_dir_alloc = 0;
 	struct strbuf a_src_dir = STRBUF_INIT;
 	enum update_mode *modes, dst_mode = 0;
-	struct stat st;
+	struct stat st, dest_st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
 	struct cache_entry *ce;
@@ -304,7 +304,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 			goto act_on_entry;
 		}
 		if (S_ISDIR(st.st_mode)
-		    && lstat(dst, &st) == 0) {
+		    && lstat(dst, &dest_st) == 0) {
 			bad = _("cannot move directory over file");
 			goto act_on_entry;
 		}
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 898a9205328..f136ea76f7f 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -174,6 +174,13 @@ test_expect_success 'do not move directory over existing directory' '
 	test_must_fail git mv path2 path0
 '
 
+test_expect_success 'rename directory to non-existing directory' '
+	mkdir dir-a &&
+	>dir-a/f &&
+	git add dir-a &&
+	git mv dir-a non-existing-dir
+'
+
 test_expect_success 'move into "."' '
 	git mv path1/path2/ .
 '

base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
-- 
gitgitgadget