Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Andrea Stacchiotti]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

Dear maintainer(s),

the following script, when executed with git 2.19 triggers the bug in
the subject line.
The problem seems to be the interaction between add -N and rename detection.

The git binary used is the one currently packaged in Debian unstable.

I have searched the list for the bug text and have found nothing,
apologies if the bug is already known.

System information, script content and script output follow.

Andrea Stacchiotti

--------------------------

andreas@trelitri:/tmp$ uname -a
Linux trelitri 4.17.0-3-amd64 #1 SMP Debian 4.17.17-1 (2018-08-18)
x86_64 GNU/Linux
andreas@trelitri:/tmp$ git --version
git version 2.19.0

andreas@trelitri:/tmp$ cat bugscript.sh
# Make a test repo
git init testrepo
cd testrepo
git config user.name A
git config user.email B

# Add a file called orig
echo 'a' > orig
git add orig
git commit -m'orig'

# Copy orig in new and modify orig
cp orig new
echo 'b' > orig

# add -N and then commit trigger the bug
git add -N new
git commit

# Cleanup
cd ..
rm -rf testrepo

andreas@trelitri:/tmp$ LANG=C ./bugscript.sh
Initialized empty Git repository in /tmp/testrepo/.git/
[master (root-commit) 5dedf30] orig
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 orig
BUG: wt-status.c:476: multiple renames on the same target? how?
./bugscript.sh: line 18: 22762 Aborted                 git commit

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 19043 bytes --]

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 26 2018, Andrea Stacchiotti wrote:

> Dear maintainer(s),
>
> the following script, when executed with git 2.19 triggers the bug in
> the subject line.
> The problem seems to be the interaction between add -N and rename detection.
>
> The git binary used is the one currently packaged in Debian unstable.
>
> I have searched the list for the bug text and have found nothing,
> apologies if the bug is already known.
>
> System information, script content and script output follow.
>
> Andrea Stacchiotti
>
> --------------------------
>
> andreas@trelitri:/tmp$ uname -a
> Linux trelitri 4.17.0-3-amd64 #1 SMP Debian 4.17.17-1 (2018-08-18)
> x86_64 GNU/Linux
> andreas@trelitri:/tmp$ git --version
> git version 2.19.0
>
> andreas@trelitri:/tmp$ cat bugscript.sh
> # Make a test repo
> git init testrepo
> cd testrepo
> git config user.name A
> git config user.email B
>
> # Add a file called orig
> echo 'a' > orig
> git add orig
> git commit -m'orig'
>
> # Copy orig in new and modify orig
> cp orig new
> echo 'b' > orig
>
> # add -N and then commit trigger the bug
> git add -N new
> git commit
>
> # Cleanup
> cd ..
> rm -rf testrepo
>
> andreas@trelitri:/tmp$ LANG=C ./bugscript.sh
> Initialized empty Git repository in /tmp/testrepo/.git/
> [master (root-commit) 5dedf30] orig
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 orig
> BUG: wt-status.c:476: multiple renames on the same target? how?
> ./bugscript.sh: line 18: 22762 Aborted                 git commit

I can't reproduce this on Debian AMD64 either 2.19.0 in unstable, or
2.19.0.605.g01d371f741 in experimental. I tried moving my ~/.gitconfig
out of the way, do you have some config options there that might be
contributing to this?

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Andrea Stacchiotti]

[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]

I'm very sorry, I indeed forgot the `diff.renames=copies`.

The following script can reproduce the bug even with a blank config:

---------------------

# Make a test repo
git init testrepo
cd testrepo
git config user.name A
git config user.email B
git config diff.renames copies

# Add a file called orig
echo 'a' > orig
git add orig
git commit -m'orig'

# Copy orig in new and modify orig
cp orig new
echo 'b' > orig

# add -N and then commit trigger the bug
git add -N new
git commit

# Cleanup
cd ..
rm -rf testrepo

Il 26/09/18 22:56, Ævar Arnfjörð Bjarmason ha scritto:
> 
> On Wed, Sep 26 2018, Andrea Stacchiotti wrote:
> 
>> Dear maintainer(s),
>>
>> the following script, when executed with git 2.19 triggers the bug in
>> the subject line.
>> The problem seems to be the interaction between add -N and rename detection.
>>
>> The git binary used is the one currently packaged in Debian unstable.
>>
>> I have searched the list for the bug text and have found nothing,
>> apologies if the bug is already known.
>>
>> System information, script content and script output follow.
>>
>> Andrea Stacchiotti
>>
>> --------------------------
>>
>> andreas@trelitri:/tmp$ uname -a
>> Linux trelitri 4.17.0-3-amd64 #1 SMP Debian 4.17.17-1 (2018-08-18)
>> x86_64 GNU/Linux
>> andreas@trelitri:/tmp$ git --version
>> git version 2.19.0
>>
>> andreas@trelitri:/tmp$ cat bugscript.sh
>> # Make a test repo
>> git init testrepo
>> cd testrepo
>> git config user.name A
>> git config user.email B
>>
>> # Add a file called orig
>> echo 'a' > orig
>> git add orig
>> git commit -m'orig'
>>
>> # Copy orig in new and modify orig
>> cp orig new
>> echo 'b' > orig
>>
>> # add -N and then commit trigger the bug
>> git add -N new
>> git commit
>>
>> # Cleanup
>> cd ..
>> rm -rf testrepo
>>
>> andreas@trelitri:/tmp$ LANG=C ./bugscript.sh
>> Initialized empty Git repository in /tmp/testrepo/.git/
>> [master (root-commit) 5dedf30] orig
>>  1 file changed, 0 insertions(+), 0 deletions(-)
>>  create mode 100644 orig
>> BUG: wt-status.c:476: multiple renames on the same target? how?
>> ./bugscript.sh: line 18: 22762 Aborted                 git commit
> 
> I can't reproduce this on Debian AMD64 either 2.19.0 in unstable, or
> 2.19.0.605.g01d371f741 in experimental. I tried moving my ~/.gitconfig
> out of the way, do you have some config options there that might be
> contributing to this?
> 

[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 19043 bytes --]

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Elijah Newren]

On Wed, Sep 26, 2018, 2:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Wed, Sep 26 2018, Andrea Stacchiotti wrote:
>
> > I'm very sorry, I indeed forgot the `diff.renames=copies`.
> >
> > The following script can reproduce the bug even with a blank config:

Thanks for the bug report and the simple testcase.

> > ---------------------
> >
> > # Make a test repo
> > git init testrepo
> > cd testrepo
> > git config user.name A
> > git config user.email B
> > git config diff.renames copies
> >
> > # Add a file called orig
> > echo 'a' > orig
> > git add orig
> > git commit -m'orig'
> >
> > # Copy orig in new and modify orig
> > cp orig new
> > echo 'b' > orig
> >
> > # add -N and then commit trigger the bug
> > git add -N new
> > git commit
> >
> > # Cleanup
> > cd ..
> > rm -rf testrepo
>
> Thanks. Bisecting shows that the bug is in dc6b1d92ca ("wt-status: use
> settings from git_diff_ui_config", 2018-05-04) first released with
> 2.18.0.

The bisect is slightly misleading; the bug was introduced in 2.17.0
for renames, and when copy detection became a thing in 2.18.0 it also
incidentally would trigger with copies.  I'll post a patch soon.

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Elijah Newren]

On Wed, Sep 26, 2018 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:
> On Wed, Sep 26, 2018, 2:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > On Wed, Sep 26 2018, Andrea Stacchiotti wrote:
> >
> > > I'm very sorry, I indeed forgot the `diff.renames=copies`.
> > >
> > > The following script can reproduce the bug even with a blank config:
>
> Thanks for the bug report and the simple testcase.
>
> > > ---------------------
> > >
> > > # Make a test repo
> > > git init testrepo
> > > cd testrepo
> > > git config user.name A
> > > git config user.email B
> > > git config diff.renames copies
> > >
> > > # Add a file called orig
> > > echo 'a' > orig
> > > git add orig
> > > git commit -m'orig'
> > >
> > > # Copy orig in new and modify orig
> > > cp orig new
> > > echo 'b' > orig
> > >
> > > # add -N and then commit trigger the bug
> > > git add -N new
> > > git commit
> > >
> > > # Cleanup
> > > cd ..
> > > rm -rf testrepo
> >
> > Thanks. Bisecting shows that the bug is in dc6b1d92ca ("wt-status: use
> > settings from git_diff_ui_config", 2018-05-04) first released with
> > 2.18.0.
>
> The bisect is slightly misleading; the bug was introduced in 2.17.0
> for renames, and when copy detection became a thing in 2.18.0 it also
> incidentally would trigger with copies.  I'll post a patch soon.

-- 8< --
Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same
 target? how?'

builtin/commit.c:prepare_to_commit() can call run_status() twice if
using the editor, including status, and the user attempts to record a
non-merge empty commit without explicit --allow-empty.  If there is also
a rename involved as well (due to using 'git add -N'), then a BUG in
wt-status.c is triggered:

  BUG: wt-status.c:476: multiple renames on the same target? how?

The reason we hit this bug is that both run_status() calls use the same
struct wt_status * (named s), and s->change is not freed between runs.
Changes are inserted into s with string_list_insert, which usually means
that the second run just recomputes all the same results and overwrites
what was computed the first time.  However, ever since commit
176ea7479309 ("wt-status.c: handle worktree renames", 2017-12-27),
wt-status started checking for renames and copies but also added a
preventative check that d->rename_status wasn't already set and output a
BUG message if it was.  The problem isn't that there are multiple rename
targets to a single path as the error implies, the problem is that 's'
is not freed/cleared between the two run_status() calls.

Ever since commit dc6b1d92ca9c ("wt-status: use settings from
git_diff_ui_config", 2018-05-04), which stopped hardcoding
DIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug
has also been triggerable with a copy instead of a rename.

Fix the bug by clearing s->change.  A better change might be to clean up
all of s between the two run_status() calls.  A good first step towards
such a goal might be writing a function to free the necessary fields in
the wt_status * struct; a cursory glance at the code suggests all of its
allocated data is probably leaked.  However, doing all that cleanup is a
bigger task for someone else interested to tackle; just fix the bug for
now.

Reported-by: Andrea Stacchiotti <andreastacchiotti@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/commit.c  |  1 +
 t/t7500-commit.sh | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index b57d8e4b82..ee98d703f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -921,6 +921,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (!commitable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
 		s->display_comment_prefix = old_display_comment_prefix;
+		string_list_clear(&s->change, 1);
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..387637b9b0 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '
 	test_i18ncmp expected-template editor-input
 '
 
+test_expect_success 'setup empty commit with unstaged rename and copy' '
+	test_create_repo unstaged_rename_and_copy &&
+	(
+		cd unstaged_rename_and_copy &&
+
+		echo content >orig &&
+		git add orig &&
+		test_commit orig &&
+
+		cp -a orig new_copy &&
+		mv orig new_rename &&
+		git add -N new_copy new_rename
+	)
+'
+
+test_expect_success 'check commit with unstaged rename and copy' '
+	(
+		cd unstaged_rename_and_copy &&
+
+		test_must_fail git -c diff.renames=copy commit
+	)
+'
+
 test_done
-- 
2.19.0.222.g923d35e14f

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Eric Sunshine]

On Thu, Sep 27, 2018 at 3:20 AM Elijah Newren <newren@gmail.com> wrote:
> Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same
>  target? how?'
> [...]
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
> @@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '
> +test_expect_success 'setup empty commit with unstaged rename and copy' '
> +       test_create_repo unstaged_rename_and_copy &&
> +       (
> +               cd unstaged_rename_and_copy &&
> +
> +               echo content >orig &&
> +               git add orig &&
> +               test_commit orig &&
> +
> +               cp -a orig new_copy &&

Non-portable -a option. Not in POSIX[1].

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html

> +               mv orig new_rename &&
> +               git add -N new_copy new_rename
> +       )
> +'

Re: Triggering "BUG: wt-status.c:476: multiple renames on the same target? how?"

[Duy Nguyen]

On Thu, Sep 27, 2018 at 9:20 AM Elijah Newren <newren@gmail.com> wrote:
> Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same
>  target? how?'
>
> builtin/commit.c:prepare_to_commit() can call run_status() twice if
> using the editor, including status, and the user attempts to record a
> non-merge empty commit without explicit --allow-empty.  If there is also
> a rename involved as well (due to using 'git add -N'), then a BUG in
> wt-status.c is triggered:
>
>   BUG: wt-status.c:476: multiple renames on the same target? how?
>
> The reason we hit this bug is that both run_status() calls use the same
> struct wt_status * (named s), and s->change is not freed between runs.
> Changes are inserted into s with string_list_insert, which usually means
> that the second run just recomputes all the same results and overwrites
> what was computed the first time.  However, ever since commit
> 176ea7479309 ("wt-status.c: handle worktree renames", 2017-12-27),
> wt-status started checking for renames and copies but also added a
> preventative check that d->rename_status wasn't already set and output a
> BUG message if it was.  The problem isn't that there are multiple rename
> targets to a single path as the error implies, the problem is that 's'
> is not freed/cleared between the two run_status() calls.

Phew.. so technically it's not my fault, I just helped expose the bug
with my BUG() line, probably.

> Ever since commit dc6b1d92ca9c ("wt-status: use settings from
> git_diff_ui_config", 2018-05-04), which stopped hardcoding
> DIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug
> has also been triggerable with a copy instead of a rename.
>
> Fix the bug by clearing s->change.  A better change might be to clean up
> all of s between the two run_status() calls.

You clear s->change just before the second call, but perhaps you
should do it right after the first. It seems other code authors were
already aware of this sharing "s" and worked around changing
s->use_color at the first call site, it seems neater to keep all this
temporary fixes in just one place:

    saved_color_setting = s->use_color;
    s->use_color = 0;
    commitable = run_status(s->fp, index_file, prefix, 1, s);
    s->use_color = saved_color_setting;

> A good first step towards
> such a goal might be writing a function to free the necessary fields in
> the wt_status * struct; a cursory glance at the code suggests all of its
> allocated data is probably leaked.  However, doing all that cleanup is a
> bigger task for someone else interested to tackle; just fix the bug for
> now.

I agree. Keep the bug fix to the point. Cleanup and stuff could be
done later (and perhaps try to run all the heavy "diff" just once
instead of twice like this).
-- 
Duy