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

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

[PATCHv2] commit: fix erroneous BUG, 'multiple renames on the same target? how?'

[Elijah Newren]

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

Changes since v1:
  - Remove non-portable and unneccessary -a flag to cp (Flagged by Eric)
  - Move the string_list_clear() from just before the second run_status()
    call to just after the first one (As suggested by Duy)

 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..342cc9cff0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -874,6 +874,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		s->use_color = 0;
 		commitable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
+		string_list_clear(&s->change, 1);
 	} else {
 		struct object_id oid;
 		const char *parent = "HEAD";
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..31ab608b67 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 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.gfbc440e6cd.dirty