{"thread":{"id":"49435","subject":"Re: Triggering \"BUG: wt-status.c:476: multiple renames on the same target? how?\"","startedAt":"2018-09-27T07:20:52Z","lastAt":"2018-09-27T17:37:07Z","messageCount":4,"participants":["Elijah Newren","Eric Sunshine","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"359037","messageId":"20180927072043.19130-1-newren@gmail.com","threadId":"49435","inReplyTo":null,"subject":"Re: Triggering \"BUG: wt-status.c:476: multiple renames on the same target? how?\"","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-27T07:20:43Z","receivedAt":"2018-09-27T07:20:52Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 26, 2018 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:\n> On Wed, Sep 26, 2018, 2:27 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> > On Wed, Sep 26 2018, Andrea Stacchiotti wrote:\n> >\n> > > I'm very sorry, I indeed forgot the `diff.renames=copies`.\n> > >\n> > > The following script can reproduce the bug even with a blank config:\n>\n> Thanks for the bug report and the simple testcase.\n>\n> > > ---------------------\n> > >\n> > > # Make a test repo\n> > > git init testrepo\n> > > cd testrepo\n> > > git config user.name A\n> > > git config user.email B\n> > > git config diff.renames copies\n> > >\n> > > # Add a file called orig\n> > > echo 'a' > orig\n> > > git add orig\n> > > git commit -m'orig'\n> > >\n> > > # Copy orig in new and modify orig\n> > > cp orig new\n> > > echo 'b' > orig\n> > >\n> > > # add -N and then commit trigger the bug\n> > > git add -N new\n> > > git commit\n> > >\n> > > # Cleanup\n> > > cd ..\n> > > rm -rf testrepo\n> >\n> > Thanks. Bisecting shows that the bug is in dc6b1d92ca (\"wt-status: use\n> > settings from git_diff_ui_config\", 2018-05-04) first released with\n> > 2.18.0.\n>\n> The bisect is slightly misleading; the bug was introduced in 2.17.0\n> for renames, and when copy detection became a thing in 2.18.0 it also\n> incidentally would trigger with copies.  I'll post a patch soon.\n\n-- 8< --\nSubject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same\n target? how?'\n\nbuiltin/commit.c:prepare_to_commit() can call run_status() twice if\nusing the editor, including status, and the user attempts to record a\nnon-merge empty commit without explicit --allow-empty.  If there is also\na rename involved as well (due to using 'git add -N'), then a BUG in\nwt-status.c is triggered:\n\n  BUG: wt-status.c:476: multiple renames on the same target? how?\n\nThe reason we hit this bug is that both run_status() calls use the same\nstruct wt_status * (named s), and s->change is not freed between runs.\nChanges are inserted into s with string_list_insert, which usually means\nthat the second run just recomputes all the same results and overwrites\nwhat was computed the first time.  However, ever since commit\n176ea7479309 (\"wt-status.c: handle worktree renames\", 2017-12-27),\nwt-status started checking for renames and copies but also added a\npreventative check that d->rename_status wasn't already set and output a\nBUG message if it was.  The problem isn't that there are multiple rename\ntargets to a single path as the error implies, the problem is that 's'\nis not freed/cleared between the two run_status() calls.\n\nEver since commit dc6b1d92ca9c (\"wt-status: use settings from\ngit_diff_ui_config\", 2018-05-04), which stopped hardcoding\nDIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug\nhas also been triggerable with a copy instead of a rename.\n\nFix the bug by clearing s->change.  A better change might be to clean up\nall of s between the two run_status() calls.  A good first step towards\nsuch a goal might be writing a function to free the necessary fields in\nthe wt_status * struct; a cursory glance at the code suggests all of its\nallocated data is probably leaked.  However, doing all that cleanup is a\nbigger task for someone else interested to tackle; just fix the bug for\nnow.\n\nReported-by: Andrea Stacchiotti <andreastacchiotti@gmail.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/commit.c  |  1 +\n t/t7500-commit.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b57d8e4b82..ee98d703f2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -921,6 +921,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (!commitable && whence != FROM_MERGE && !allow_empty &&\n \t    !(amend && is_a_merge(current_head))) {\n \t\ts->display_comment_prefix = old_display_comment_prefix;\n+\t\tstring_list_clear(&s->change, 1);\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 170b4810e0..387637b9b0 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '\n \ttest_i18ncmp expected-template editor-input\n '\n \n+test_expect_success 'setup empty commit with unstaged rename and copy' '\n+\ttest_create_repo unstaged_rename_and_copy &&\n+\t(\n+\t\tcd unstaged_rename_and_copy &&\n+\n+\t\techo content >orig &&\n+\t\tgit add orig &&\n+\t\ttest_commit orig &&\n+\n+\t\tcp -a orig new_copy &&\n+\t\tmv orig new_rename &&\n+\t\tgit add -N new_copy new_rename\n+\t)\n+'\n+\n+test_expect_success 'check commit with unstaged rename and copy' '\n+\t(\n+\t\tcd unstaged_rename_and_copy &&\n+\n+\t\ttest_must_fail git -c diff.renames=copy commit\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.222.g923d35e14f\n\n"},{"id":"359040","messageId":"CAPig+cRW1S78Oiq9RPZjOBC651DO3=8Uzg69sji+x1QpOeH4ig@mail.gmail.com","threadId":"49435","inReplyTo":"20180927072043.19130-1-newren@gmail.com","subject":"Re: Triggering \"BUG: wt-status.c:476: multiple renames on the same target? how?\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-27T10:15:45Z","receivedAt":"2018-09-27T10:15:58Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 27, 2018 at 3:20 AM Elijah Newren <newren@gmail.com> wrote:\n> Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same\n>  target? how?'\n> [...]\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n> diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\n> @@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '\n> +test_expect_success 'setup empty commit with unstaged rename and copy' '\n> +       test_create_repo unstaged_rename_and_copy &&\n> +       (\n> +               cd unstaged_rename_and_copy &&\n> +\n> +               echo content >orig &&\n> +               git add orig &&\n> +               test_commit orig &&\n> +\n> +               cp -a orig new_copy &&\n\nNon-portable -a option. Not in POSIX[1].\n\n[1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html\n\n> +               mv orig new_rename &&\n> +               git add -N new_copy new_rename\n> +       )\n> +'\n"},{"id":"359069","messageId":"CACsJy8BPf+ctPs9rHeC+mf9btcPQiW35kA5T81J=66tzF67Pyw@mail.gmail.com","threadId":"49435","inReplyTo":"20180927072043.19130-1-newren@gmail.com","subject":"Re: Triggering \"BUG: wt-status.c:476: multiple renames on the same target? how?\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-27T16:48:05Z","receivedAt":"2018-09-27T16:48:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Sep 27, 2018 at 9:20 AM Elijah Newren <newren@gmail.com> wrote:\n> Subject: [PATCH] commit: fix erroneous BUG, 'multiple renames on the same\n>  target? how?'\n>\n> builtin/commit.c:prepare_to_commit() can call run_status() twice if\n> using the editor, including status, and the user attempts to record a\n> non-merge empty commit without explicit --allow-empty.  If there is also\n> a rename involved as well (due to using 'git add -N'), then a BUG in\n> wt-status.c is triggered:\n>\n>   BUG: wt-status.c:476: multiple renames on the same target? how?\n>\n> The reason we hit this bug is that both run_status() calls use the same\n> struct wt_status * (named s), and s->change is not freed between runs.\n> Changes are inserted into s with string_list_insert, which usually means\n> that the second run just recomputes all the same results and overwrites\n> what was computed the first time.  However, ever since commit\n> 176ea7479309 (\"wt-status.c: handle worktree renames\", 2017-12-27),\n> wt-status started checking for renames and copies but also added a\n> preventative check that d->rename_status wasn't already set and output a\n> BUG message if it was.  The problem isn't that there are multiple rename\n> targets to a single path as the error implies, the problem is that 's'\n> is not freed/cleared between the two run_status() calls.\n\nPhew.. so technically it's not my fault, I just helped expose the bug\nwith my BUG() line, probably.\n\n> Ever since commit dc6b1d92ca9c (\"wt-status: use settings from\n> git_diff_ui_config\", 2018-05-04), which stopped hardcoding\n> DIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug\n> has also been triggerable with a copy instead of a rename.\n>\n> Fix the bug by clearing s->change.  A better change might be to clean up\n> all of s between the two run_status() calls.\n\nYou clear s->change just before the second call, but perhaps you\nshould do it right after the first. It seems other code authors were\nalready aware of this sharing \"s\" and worked around changing\ns->use_color at the first call site, it seems neater to keep all this\ntemporary fixes in just one place:\n\n    saved_color_setting = s->use_color;\n    s->use_color = 0;\n    commitable = run_status(s->fp, index_file, prefix, 1, s);\n    s->use_color = saved_color_setting;\n\n> A good first step towards\n> such a goal might be writing a function to free the necessary fields in\n> the wt_status * struct; a cursory glance at the code suggests all of its\n> allocated data is probably leaked.  However, doing all that cleanup is a\n> bigger task for someone else interested to tackle; just fix the bug for\n> now.\n\nI agree. Keep the bug fix to the point. Cleanup and stuff could be\ndone later (and perhaps try to run all the heavy \"diff\" just once\ninstead of twice like this).\n-- \nDuy\n"},{"id":"359074","messageId":"20180927173657.4723-1-newren@gmail.com","threadId":"49435","inReplyTo":"20180927072043.19130-1-newren@gmail.com","subject":"[PATCHv2] commit: fix erroneous BUG, 'multiple renames on the same target? how?'","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-09-27T17:36:57Z","receivedAt":"2018-09-27T17:37:07Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"builtin/commit.c:prepare_to_commit() can call run_status() twice if\nusing the editor, including status, and the user attempts to record a\nnon-merge empty commit without explicit --allow-empty.  If there is also\na rename involved as well (due to using 'git add -N'), then a BUG in\nwt-status.c is triggered:\n\n  BUG: wt-status.c:476: multiple renames on the same target? how?\n\nThe reason we hit this bug is that both run_status() calls use the same\nstruct wt_status * (named s), and s->change is not freed between runs.\nChanges are inserted into s with string_list_insert, which usually means\nthat the second run just recomputes all the same results and overwrites\nwhat was computed the first time.  However, ever since commit\n176ea7479309 (\"wt-status.c: handle worktree renames\", 2017-12-27),\nwt-status started checking for renames and copies but also added a\npreventative check that d->rename_status wasn't already set and output a\nBUG message if it was.  The problem isn't that there are multiple rename\ntargets to a single path as the error implies, the problem is that 's'\nis not freed/cleared between the two run_status() calls.\n\nEver since commit dc6b1d92ca9c (\"wt-status: use settings from\ngit_diff_ui_config\", 2018-05-04), which stopped hardcoding\nDIFF_DETECT_RENAME and allowed users to ask for copy detection, this bug\nhas also been triggerable with a copy instead of a rename.\n\nFix the bug by clearing s->change.  A better change might be to clean up\nall of s between the two run_status() calls.  A good first step towards\nsuch a goal might be writing a function to free the necessary fields in\nthe wt_status * struct; a cursory glance at the code suggests all of its\nallocated data is probably leaked.  However, doing all that cleanup is a\nbigger task for someone else interested to tackle; just fix the bug for\nnow.\n\nReported-by: Andrea Stacchiotti <andreastacchiotti@gmail.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n\nChanges since v1:\n  - Remove non-portable and unneccessary -a flag to cp (Flagged by Eric)\n  - Move the string_list_clear() from just before the second run_status()\n    call to just after the first one (As suggested by Duy)\n\n builtin/commit.c  |  1 +\n t/t7500-commit.sh | 23 +++++++++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b57d8e4b82..342cc9cff0 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -874,6 +874,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\ts->use_color = 0;\n \t\tcommitable = run_status(s->fp, index_file, prefix, 1, s);\n \t\ts->use_color = saved_color_setting;\n+\t\tstring_list_clear(&s->change, 1);\n \t} else {\n \t\tstruct object_id oid;\n \t\tconst char *parent = \"HEAD\";\ndiff --git a/t/t7500-commit.sh b/t/t7500-commit.sh\nindex 170b4810e0..31ab608b67 100755\n--- a/t/t7500-commit.sh\n+++ b/t/t7500-commit.sh\n@@ -359,4 +359,27 @@ test_expect_success 'new line found before status message in commit template' '\n \ttest_i18ncmp expected-template editor-input\n '\n \n+test_expect_success 'setup empty commit with unstaged rename and copy' '\n+\ttest_create_repo unstaged_rename_and_copy &&\n+\t(\n+\t\tcd unstaged_rename_and_copy &&\n+\n+\t\techo content >orig &&\n+\t\tgit add orig &&\n+\t\ttest_commit orig &&\n+\n+\t\tcp orig new_copy &&\n+\t\tmv orig new_rename &&\n+\t\tgit add -N new_copy new_rename\n+\t)\n+'\n+\n+test_expect_success 'check commit with unstaged rename and copy' '\n+\t(\n+\t\tcd unstaged_rename_and_copy &&\n+\n+\t\ttest_must_fail git -c diff.renames=copy commit\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.222.gfbc440e6cd.dirty\n\n"}]}