[BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

[BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

[Kristoffer Haugsbakk]

You can trigger an assertion by giving these arguments to `git mv`:

    <dir>/file <dir> <other dir>

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)

```
git config --global --add safe.directory /tmp
dir=$(mktemp -d)
cd $dir
git init
mkdir a
touch a/a.txt
git add a/a.txt
git commit -m 'init'
mkdir b
# Assertion triggered
git mv a/a.txt a b
# `.git/index.lock` still lingers after this; commands like `git add
# <file>` will fail
```

The output:

```
git: builtin/mv.c:481: cmd_mv: Assertion `pos >= 0' failed.
Aborted (core dumped)
```

Also `.git/index.lock` is still there.

> What did you expect to happen? (Expected behavior)

A normal error message if the command is nonsensical (I don’t know; that’s
not the point). Also `.git/index.lock` to be cleaned up.

> What happened instead? (Actual behavior)

An assertion failed. `.git/index.lock` is not cleaned up.

> What's different between what you expected and what actually happened?

See above.

> Anything else you want to add:

Same behavior on `master` (a26002b628 (The fifth batch, 2024-01-02)).

```
./bin-wrappers/git config --global --add safe.directory /tmp
dir=$(mktemp -d)
./bin-wrappers/git -C $dir init
mkdir $dir/a
touch $dir/a/a.txt
./bin-wrappers/git -C $dir add $dir/a/a.txt
./bin-wrappers/git -C $dir commit -m 'init'
mkdir $dir/b
# Assertion triggered
./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b
```

> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.43.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.2.0-39-generic #40~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu Nov 16 10:53:04 UTC 2 x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]

-- 
Kristoffer Haugsbakk

Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> You can trigger an assertion by giving these arguments to `git mv`:
>
>     <dir>/file <dir> <other dir>
> ...
>> What did you expect to happen? (Expected behavior)
>
> A normal error message if the command is nonsensical (I don’t know; that’s
> not the point). Also `.git/index.lock` to be cleaned up.

Good find.

Not just that, but when the command fails in the middle like this,
it leaves the working tree in a half-updated state, i.e.

> ./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b

will first move a/a.txt to b/a.txt, then try to move a (actually,
all contents of it, including a/a.txt) to b/a and finds that "the
command is nonsensical" and aborts, and by that time, there is no
a/a.txt (i.e. the working tree has been modified).  The failure
should be made atomic, just like "git switch" to another branch may
stop _without_ touching anything in the working tree when it may
have to fail (e.g., due to a file being dirty).

Thanks for reporting, Kristoffer.

Any takers?

$ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 builtin/mv.c
    15	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
    10	Elijah Newren <newren@gmail.com>
     5	Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     2	Junio C Hamano <gitster@pobox.com>
     1	Andrzej Hunt <ajrhunt@google.com>
     1	Calvin Wan <calvinwan@google.com>
     1	Derrick Stolee <stolee@gmail.com>
     1	Sebastian Thiel <sebastian.thiel@icloud.com>
     1	Torsten Bögershausen <tboegi@web.de>

Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

[Dragan Simic]

On 2024-01-05 19:52, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> 
>> You can trigger an assertion by giving these arguments to `git mv`:
>> 
>>     <dir>/file <dir> <other dir>
>> ...
>>> What did you expect to happen? (Expected behavior)
>> 
>> A normal error message if the command is nonsensical (I don’t know; 
>> that’s
>> not the point). Also `.git/index.lock` to be cleaned up.
> 
> Good find.

Yes, thanks to Kristoffer for reporting this issue.

> Not just that, but when the command fails in the middle like this,
> it leaves the working tree in a half-updated state, i.e.
> 
>> ./bin-wrappers/git -C $dir mv $dir/a/a.txt $dir/a $dir/b
> 
> will first move a/a.txt to b/a.txt, then try to move a (actually,
> all contents of it, including a/a.txt) to b/a and finds that "the
> command is nonsensical" and aborts, and by that time, there is no
> a/a.txt (i.e. the working tree has been modified).  The failure
> should be made atomic, just like "git switch" to another branch may
> stop _without_ touching anything in the working tree when it may
> have to fail (e.g., due to a file being dirty).
> 
> Thanks for reporting, Kristoffer.
> 
> Any takers?

This looks like a rather interesting bugfix to me. :)  Though, I've 
unfortunately contracted some _nasty_ flu, so I'm still simply unable to 
work on pretty much anything in the next 5-6 days or so, at which point 
I hope to be operational again.

Thus, unless someone else can get it done faster, I should be able to 
start working on it in about a week or so.  Hopefully, that is.

> $ git shortlog --since=3.years -s -n -e --no-merges v2.43.0 
> builtin/mv.c
>     15	Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
>     10	Elijah Newren <newren@gmail.com>
>      5	Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>      2	Junio C Hamano <gitster@pobox.com>
>      1	Andrzej Hunt <ajrhunt@google.com>
>      1	Calvin Wan <calvinwan@google.com>
>      1	Derrick Stolee <stolee@gmail.com>
>      1	Sebastian Thiel <sebastian.thiel@icloud.com>
>      1	Torsten Bögershausen <tboegi@web.de>

Re: [BUG] mv: can trigger assertion failure with three parameters (builtin/mv.c:481)

[Kristoffer Haugsbakk]

Here’s a failing test. This fails on top of `master` (3e0d3cd5c7 (Merge branch
'jx/dirstat-parseopt-help', 2024-02-15)).

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

-- >8 --
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 879a6dce601..4f180903486 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -549,4 +549,16 @@ test_expect_success 'moving nested submodules' '
 	git status
 '
 
+test_expect_success '(TODO title) nonsense move' '
+	test_when_finished git reset --hard HEAD &&
+	git reset --hard HEAD &&
+	mkdir -p a &&
+	mkdir -p b &&
+	>a/a.txt &&
+	git add a/a.txt &&
+	test_must_fail git mv a/a.txt a b &&
+	git status --porcelain >actual &&
+	grep "^A[ ]*a/a.txt$" actual
+'
+
 test_done

[PATCH v2] t7001: add failure test which triggers assertion

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

git-mv(1) gets very confused:

    git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.
    Aborted (core dumped)
    test_must_fail: died by signal 6: git mv a/a.txt a b
    fatal: Unable to create '<path>.git/index.lock': File exists.

    Another git process seems to be running in this repository, e.g.
    an editor opened by 'git commit'. Please make sure all processes
    are terminated then try again. If it still fails, a git process
    may have crashed in this repository earlier:
    remove the file manually to continue.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    It’s been a good while.  Let’s just add this as a known breakage?

 t/t7001-mv.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 86258f9f430..739c25e2551 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -551,4 +551,16 @@ test_expect_success 'moving nested submodules' '
 	git status
 '
 
+test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '
+	test_when_finished git reset --hard HEAD &&
+	git reset --hard HEAD &&
+	mkdir -p a &&
+	mkdir -p b &&
+	>a/a.txt &&
+	git add a/a.txt &&
+	test_must_fail git mv a/a.txt a b &&
+	git status --porcelain >actual &&
+	grep "^A[ ]*a/a.txt$" actual
+'
+
 test_done

base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
-- 
2.46.1.641.g54e7913fcb6

Re: [PATCH v2] t7001: add failure test which triggers assertion

[Taylor Blau]

On Mon, Oct 21, 2024 at 12:14:46AM +0200, kristofferhaugsbakk@fastmail.com wrote:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> git-mv(1) gets very confused:
>
>     git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.
>     Aborted (core dumped)
>     test_must_fail: died by signal 6: git mv a/a.txt a b
>     fatal: Unable to create '<path>.git/index.lock': File exists.
>
>     Another git process seems to be running in this repository, e.g.
>     an editor opened by 'git commit'. Please make sure all processes
>     are terminated then try again. If it still fails, a git process
>     may have crashed in this repository earlier:
>     remove the file manually to continue.

There was some good analysis of what the problem was in the earlier
parts of this thread. I think it is probably worth capturing some of
those here, too.

> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>
> Notes (series):
>     It’s been a good while.  Let’s just add this as a known breakage?
>
>  t/t7001-mv.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 86258f9f430..739c25e2551 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -551,4 +551,16 @@ test_expect_success 'moving nested submodules' '
>  	git status
>  '
>
> +test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '

Do we want to be so specific about the line number that the assertion
failure occurs on? The actual coredump triggered by this test will tell
us that information. But in the meantime this line is likely to go stale
as builtin/mv.c changes over time.

> +	test_when_finished git reset --hard HEAD &&
> +	git reset --hard HEAD &&
> +	mkdir -p a &&
> +	mkdir -p b &&
> +	>a/a.txt &&
> +	git add a/a.txt &&
> +	test_must_fail git mv a/a.txt a b &&
> +	git status --porcelain >actual &&
> +	grep "^A[ ]*a/a.txt$" actual
> +'

Thanks,
Taylor

Re: [PATCH v2] t7001: add failure test which triggers assertion

[Kristoffer Haugsbakk]

On Mon, Oct 21, 2024, at 23:21, Taylor Blau wrote:
> On Mon, Oct 21, 2024 at 12:14:46AM +0200,
> kristofferhaugsbakk@fastmail.com wrote:
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> git-mv(1) gets very confused:
>>
>>     git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.
>>     Aborted (core dumped)
>>     test_must_fail: died by signal 6: git mv a/a.txt a b
>>     fatal: Unable to create '<path>.git/index.lock': File exists.
>>
>>     Another git process seems to be running in this repository, e.g.
>>     an editor opened by 'git commit'. Please make sure all processes
>>     are terminated then try again. If it still fails, a git process
>>     may have crashed in this repository earlier:
>>     remove the file manually to continue.
>
> There was some good analysis of what the problem was in the earlier
> parts of this thread. I think it is probably worth capturing some of
> those here, too.

I will try to incorporate Junio’s analysis into the commit message
tomorrow.  :)

>> +test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '
>
> Do we want to be so specific about the line number that the assertion
> failure occurs on? The actual coredump triggered by this test will tell
> us that information. But in the meantime this line is likely to go stale
> as builtin/mv.c changes over time.

You’re right, it’s overly specific/volatile.

Thanks

-- 
Kristoffer Haugsbakk

Re: [PATCH v2] t7001: add failure test which triggers assertion

[Taylor Blau]

On Mon, Oct 21, 2024 at 11:25:26PM +0200, Kristoffer Haugsbakk wrote:
> On Mon, Oct 21, 2024, at 23:21, Taylor Blau wrote:
> > On Mon, Oct 21, 2024 at 12:14:46AM +0200,
> > kristofferhaugsbakk@fastmail.com wrote:
> >> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
> >>
> >> git-mv(1) gets very confused:
> >>
> >>     git: builtin/mv.c:506: cmd_mv: Assertion `pos >= 0' failed.
> >>     Aborted (core dumped)
> >>     test_must_fail: died by signal 6: git mv a/a.txt a b
> >>     fatal: Unable to create '<path>.git/index.lock': File exists.
> >>
> >>     Another git process seems to be running in this repository, e.g.
> >>     an editor opened by 'git commit'. Please make sure all processes
> >>     are terminated then try again. If it still fails, a git process
> >>     may have crashed in this repository earlier:
> >>     remove the file manually to continue.
> >
> > There was some good analysis of what the problem was in the earlier
> > parts of this thread. I think it is probably worth capturing some of
> > those here, too.
>
> I will try to incorporate Junio’s analysis into the commit message
> tomorrow.  :)

Thanks, I look forward to it!

Thanks,
Taylor

[PATCH v3] t7001: add failure test which triggers assertion

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

`git mv a/a.txt a b/` is a nonsense instruction.  Instead of failing
gracefully the command trips over itself,[1] leaving behind unfinished
work:

1. first it moves `a/a.txt` to `b/a.txt`; then
2. tries to move `a/`, including `a/a.txt`; then
3. figures out that it’s in a bad state (assertion); and finally
4. aborts.

Now you’re left with a partially-updated index.

The command should instead fail gracefully and make no changes to the
index until it knows that it can complete a sensible action.

For now just add a failing test since this has been known about for
a while.[2]

† 1: Caused by a `pos >= 0` assertion
[2]: https://lore.kernel.org/git/d1f739fe-b28e-451f-9e01-3d2e24a0fe0d@app.fastmail.com/

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v3:
    • Rewrite commit message based on Junio’s reply
    • Tweak test description: less volatile.  Also mention index state.
    v1/v2:
    • It’s been a good while.  Let’s just add this as a known breakage?

Notes (meta-trailers):
    Helped-by: Junio C Hamano <gitster@pobox.com>
        Comment: Commit message is based on his description
        Link: https://lore.kernel.org/git/xmqqil47obnw.fsf@gitster.g/

 t/t7001-mv.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 86258f9f43..69c9190772 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -551,4 +551,16 @@ test_expect_success 'moving nested submodules' '
 	git status
 '
 
+test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '
+	test_when_finished git reset --hard HEAD &&
+	git reset --hard HEAD &&
+	mkdir -p a &&
+	mkdir -p b &&
+	>a/a.txt &&
+	git add a/a.txt &&
+	test_must_fail git mv a/a.txt a b &&
+	git status --porcelain >actual &&
+	grep "^A[ ]*a/a.txt$" actual
+'
+
 test_done

Interdiff against v2:
  diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
  index 739c25e255..69c9190772 100755
  --- a/t/t7001-mv.sh
  +++ b/t/t7001-mv.sh
  @@ -551,7 +551,7 @@ test_expect_success 'moving nested submodules' '
   	git status
   '
   
  -test_expect_failure 'nonsense mv triggers assertion failure at builtin/mv.c:502' '
  +test_expect_failure 'nonsense mv triggers assertion failure and partially updated index' '
   	test_when_finished git reset --hard HEAD &&
   	git reset --hard HEAD &&
   	mkdir -p a &&

base-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0
-- 
2.46.1.641.g54e7913fcb6

Re: [PATCH v3] t7001: add failure test which triggers assertion

[Taylor Blau]

On Tue, Oct 22, 2024 at 11:14:33PM +0200, kristofferhaugsbakk@fastmail.com wrote:
> Notes (series):
>     v3:
>     • Rewrite commit message based on Junio’s reply
>     • Tweak test description: less volatile.  Also mention index state.
>     v1/v2:
>     • It’s been a good while.  Let’s just add this as a known breakage?

Thanks, this the new version looks good to me. Let's start merging this
one down.

Thanks,
Taylor