Re: Regression in v2.23
Hi Thomas,
On Tue, 8 Oct 2019, Johannes Schindelin wrote:
> On Mon, 7 Oct 2019, Thomas Gummerer wrote:
>
> > Subject: [PATCH] range-diff: don't segfault with mode-only changes
> >
> > If we don't have a new file, deleted file or renamed file in a diff,
> > we currently add 'patch.new_name' to the range-diff header. This
> > works well for files that are changed. However if we have a pure mode
> > change, 'patch.new_name' is NULL, and thus range-diff segfaults.
> >
> > We can however rely on 'patch.def_name' in that case, which is
> > extracted from the 'diff --git' line and should be equal to
> > 'patch.new_name'. Use that instead to avoid the segfault.
> >
> > Signed-off-by: Thomas Gummerer
> > ---
> > range-diff.c | 20 ++--
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/range-diff.c b/range-diff.c
> > index ba1e9a4265..d8d906b3c6 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct
> > string_list *list)
> > if (len < 0)
> > die(_("could not parse git header '%.*s'"),
> > (int)len, line);
> > strbuf_addstr(&buf, " ## ");
> > - if (patch.is_new > 0)
> > + free(current_filename);
> > + if (patch.is_new > 0) {
> > strbuf_addf(&buf, "%s (new)", patch.new_name);
> > - else if (patch.is_delete > 0)
> > + current_filename = xstrdup(patch.new_name);
> > + } else if (patch.is_delete > 0) {
> > strbuf_addf(&buf, "%s (deleted)",
> > patch.old_name);
> > - else if (patch.is_rename)
> > - strbuf_addf(&buf, "%s => %s", patch.old_name,
> > patch.new_name);
> > - else
> > - strbuf_addstr(&buf, patch.new_name);
> > -
> > - free(current_filename);
> > - if (patch.is_delete > 0)
> > current_filename = xstrdup(patch.old_name);
> > - else
> > + } else if (patch.is_rename) {
> > + strbuf_addf(&buf, "%s => %s", patch.old_name,
> > patch.new_name);
> > current_filename = xstrdup(patch.new_name);
> > + } else {
> > + strbuf_addstr(&buf, patch.def_name);
> > + current_filename = xstrdup(patch.def_name);
> > + }
> >
> > if (patch.new_mode && patch.old_mode &&
> > patch.old_mode != patch.new_mode)
> > --
>
> I am not quite sure that this fixes it...
Whoops. I should learn to distrust `git apply` claiming success when
running in `t/`. (I tried to apply your patch, but nothing was actually
applied before I ran `make`.)
So it totally fixes the issue (feel free to just pick up the regression
test case).
Having said that, I would agree with Junio that it'd be nicer to make
`parse_git_diff_header()` more useful to all of its callers, including
future ones.
Sorry for the misreport, and thanks for all the patch,
Dscho
> Here is my regression test case:
>
> -- snipsnap --
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index ec548654ce1..6aca7f5a5b1 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as
> commentary' '
> grep "> 1: .* new message" 0001-*
> '
>
> +test_expect_success 'range-diff and mode-only changes' '
> + git switch -c mode-only &&
> +
> + test_commit mode-only &&
> +
> + : pretend it is executable &&
> + git add --chmod=+x mode-only.t &&
> + chmod a+x mode-only.t &&
> + test_tick &&
> + git commit -m mode-only &&
> +
> + git range-diff @^...
> +'
> +
> test_done
>
>
>
Re: Regression in v2.23
Hi Thomas,
On Mon, 7 Oct 2019, Thomas Gummerer wrote:
> Subject: [PATCH] range-diff: don't segfault with mode-only changes
>
> If we don't have a new file, deleted file or renamed file in a diff,
> we currently add 'patch.new_name' to the range-diff header. This
> works well for files that are changed. However if we have a pure mode
> change, 'patch.new_name' is NULL, and thus range-diff segfaults.
>
> We can however rely on 'patch.def_name' in that case, which is
> extracted from the 'diff --git' line and should be equal to
> 'patch.new_name'. Use that instead to avoid the segfault.
>
> Signed-off-by: Thomas Gummerer
> ---
> range-diff.c | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index ba1e9a4265..d8d906b3c6 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct
> string_list *list)
> if (len < 0)
> die(_("could not parse git header '%.*s'"),
> (int)len, line);
> strbuf_addstr(&buf, " ## ");
> - if (patch.is_new > 0)
> + free(current_filename);
> + if (patch.is_new > 0) {
> strbuf_addf(&buf, "%s (new)", patch.new_name);
> - else if (patch.is_delete > 0)
> + current_filename = xstrdup(patch.new_name);
> + } else if (patch.is_delete > 0) {
> strbuf_addf(&buf, "%s (deleted)",
> patch.old_name);
> - else if (patch.is_rename)
> - strbuf_addf(&buf, "%s => %s", patch.old_name,
> patch.new_name);
> - else
> - strbuf_addstr(&buf, patch.new_name);
> -
> - free(current_filename);
> - if (patch.is_delete > 0)
> current_filename = xstrdup(patch.old_name);
> - else
> + } else if (patch.is_rename) {
> + strbuf_addf(&buf, "%s => %s", patch.old_name,
> patch.new_name);
> current_filename = xstrdup(patch.new_name);
> + } else {
> + strbuf_addstr(&buf, patch.def_name);
> + current_filename = xstrdup(patch.def_name);
> + }
>
> if (patch.new_mode && patch.old_mode &&
> patch.old_mode != patch.new_mode)
> --
I am not quite sure that this fixes it... Here is my regression test case:
-- snipsnap --
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ec548654ce1..6aca7f5a5b1 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as
commentary' '
grep "> 1: .* new message" 0001-*
'
+test_expect_success 'range-diff and mode-only changes' '
+ git switch -c mode-only &&
+
+ test_commit mode-only &&
+
+ : pretend it is executable &&
+ git add --chmod=+x mode-only.t &&
+ chmod a+x mode-only.t &&
+ test_tick &&
+ git commit -m mode-only &&
+
+ git range-diff @^...
+'
+
test_done
Re: Regression in v2.23
Hi Junio, On Tue, 8 Oct 2019, Junio C Hamano wrote: > Thomas Gummerer writes: > > > We can however rely on 'patch.def_name' in that case, which is > > extracted from the 'diff --git' line and should be equal to > > 'patch.new_name'. Use that instead to avoid the segfault. > > This patch makes the way this function calls parse_git_diff_header() > more in line with the way how it is used by its original caller in > apply.c::find_header(), but not quite. > > I have to wonder if we want to move a bit of code around so that > callers of parse_git_diff_header() do not have to worry about > def_name and can rely on new_name and old_name fields correctly > filled. > > There was only one caller of the parse_git_diff_header() function > before range-diff. The division of labour between find_header() and > parse_git_diff_header() did not make any difference to the consumers > of the new/old_name fields. They only cared that they do not have > to worry about def_name. But by calling parse_git_diff_header() > that forces the caller to worry about def_name (which is done by > find_header() to free its callers from doing so), range-diff took > responsibility of caring, which was suboptimal. The interface could > have been a bit more cleaned up before we started to reuse it in the > new caller, and as this bug shows, it may be time to do so now, no? > > Perhaps before returing, parse_git_diff_header() should fill the two > names with xstrdup() of def_name if (!old_name && !new_name && > !!def_name); all other cases the existing caller and this new caller > would work unchanged correctly, no? FWIW I totally agree. Ciao, Dscho
Re: Regression in v2.23
Hello Thomas, On Mon, Oct 07, 2019 at 02:48:31PM +0100, Thomas Gummerer wrote: > I can hopefully do some more testing and write automated tests later > today or tomorrow. In the meantime it would be awesome if you could > confirm if this patch fixes the problem you were seeing. I assume you already tested on my constructed example from the report. I just tested on the original and there is works fine, too. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: Regression in v2.23
Thomas Gummerer writes: > We can however rely on 'patch.def_name' in that case, which is > extracted from the 'diff --git' line and should be equal to > 'patch.new_name'. Use that instead to avoid the segfault. This patch makes the way this function calls parse_git_diff_header() more in line with the way how it is used by its original caller in apply.c::find_header(), but not quite. I have to wonder if we want to move a bit of code around so that callers of parse_git_diff_header() do not have to worry about def_name and can rely on new_name and old_name fields correctly filled. There was only one caller of the parse_git_diff_header() function before range-diff. The division of labour between find_header() and parse_git_diff_header() did not make any difference to the consumers of the new/old_name fields. They only cared that they do not have to worry about def_name. But by calling parse_git_diff_header() that forces the caller to worry about def_name (which is done by find_header() to free its callers from doing so), range-diff took responsibility of caring, which was suboptimal. The interface could have been a bit more cleaned up before we started to reuse it in the new caller, and as this bug shows, it may be time to do so now, no? Perhaps before returing, parse_git_diff_header() should fill the two names with xstrdup() of def_name if (!old_name && !new_name && !!def_name); all other cases the existing caller and this new caller would work unchanged correctly, no?
Re: Regression in v2.23
On 10/07, Uwe Kleine-König wrote:
> Hello,
>
> With git 2.23.0 I have:
>
> uwe@taurus:~/tmp/rangediff-segfault$ git init
> Initialized empty Git repository in
> /home/uwe/tmp/rangediff-segfault/.git/
> uwe@taurus:~/tmp/rangediff-segfault$ echo root > root
> uwe@taurus:~/tmp/rangediff-segfault$ git add root
> uwe@taurus:~/tmp/rangediff-segfault$ git commit -m root
> [master (root-commit) b0feddb2dee8] root
>1 file changed, 1 insertion(+)
>create mode 100644 root
> uwe@taurus:~/tmp/rangediff-segfault$ echo content > file
> uwe@taurus:~/tmp/rangediff-segfault$ chmod +x file
> uwe@taurus:~/tmp/rangediff-segfault$ git add file
> uwe@taurus:~/tmp/rangediff-segfault$ git commit -m file
> [master 45b547c57acd] file
>1 file changed, 1 insertion(+)
>create mode 100755 file
> uwe@taurus:~/tmp/rangediff-segfault$ chmod -x file
> uwe@taurus:~/tmp/rangediff-segfault$ git add file
> uwe@taurus:~/tmp/rangediff-segfault$ git commit -m 'chmod -x'
> [master eaa5d3b98caa] chmod -x
>1 file changed, 0 insertions(+), 0 deletions(-)
>mode change 100755 => 100644 file
> uwe@taurus:~/tmp/rangediff-segfault$ git range-diff @~2..@~ @~2..
> Segmentation fault (core dumped)
>
> Bisecting points to b66885a30cb84fc61986bc4eea805a31fdbea79a, current master
> (b744c3af07a15aaeb1b82fab689995fd5528f120) segfaults in the same way.
>
> This is somehow similar to
> https://public-inbox.org/git/20190923101929.ga18...@kitsune.suse.cz/ but
> the patch by Johannes Schindelin sent in
> https://public-inbox.org/git/pull.373.git.gitgitgad...@gmail.com/
> doesn't help me.
Thanks for the report, and testing if those patches help.
> For me the segfault also happens in
>
> strbuf_addstr(&buf, patch.new_name);
>
> with patch.new_name being NULL.
>
> The matching backtrace and patch object looks as follows:
>
> (gdb) bt
> #0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
> #1 0x555cc448949c in strbuf_addstr (s=,
> sb=0x7ffcd1d9ef00)
> at strbuf.h:292
> #2 read_patches (range=range@entry=0x555cc5dc2b70 "@~2..",
> list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
> #3 0x555cc44898a8 in show_range_diff (range1=0x555cc5dc2b50
> "@~2..@~",
> range2=0x555cc5dc2b70 "@~2..", creation_factor=60, dual_color=1,
> diffopt=diffopt@entry=0x7ffcd1d9f680) at range-diff.c:507
> #4 0x555cc4397aa6 in cmd_range_diff (argc=,
> argv=, prefix=) at
> builtin/range-diff.c:80
> #5 0x555cc4328494 in run_builtin (argv=,
> argc=, p=) at git.c:445
> #6 handle_builtin (argc=, argv=) at
> git.c:674
> #7 0x555cc4329554 in run_argv (argv=0x7ffcd1d9f9e0,
> argcp=0x7ffcd1d9f9ec)
> at git.c:741
> #8 cmd_main (argc=, argv=) at git.c:872
> #9 0x555cc432803a in main (argc=4, argv=0x7ffcd1d9fc78)
> at common-main.c:52
> (gdb) up 2
> #2 read_patches (range=range@entry=0x555cc5dc2b70 "@~2..",
> list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
> 126 range-diff.c: No such file or directory.
> (gdb) print patch
> $1 = {new_name = 0x0, old_name = 0x0, def_name = 0x555cc5dc98c0 "file",
> old_mode = 33261, new_mode = 33188, is_new = 0, is_delete = 0,
> rejected = 0,
> ws_rule = 0, lines_added = 0, lines_deleted = 0, score = 0,
> extension_linenr = 0, is_toplevel_relative = 0, inaccurate_eof = 0,
> is_binary = 0, is_copy = 0, is_rename = 0, recount = 0,
> conflicted_threeway = 0, direct_to_threeway = 0, crlf_in_old = 0,
> fragments = 0x0, result = 0x0, resultsize = 0,
> old_oid_prefix = '\000' ,
> new_oid_prefix = '\000' , next = 0x0,
> threeway_stage = {{
> hash = '\000' }, {hash = '\000' times>}, {
> hash = '\000' }}}
>
> I guess you are able to work out the details with this information. If you
> need
> more input, please Cc: me on replies.
Indeed, I was able to figure out what's going wrong from the
backtrace. Here's a patch. It's not ready for inclusion, as I still
need to write some tests, and make sure I'm not breaking something
else.
I can hopefully do some more testing and write automated tests later
today or tomorrow. In the meantime it would be awesome if you could
confirm if this patch fixes the problem you were seeing.
I have pushed this to GitHub as well if you prefer that to applying
the patch yourself: https://github.com/tgummerer/git
tg/range-diff-mode-only-change
--- >8 ---
Subject: [PATCH] range-diff: don't segfault with mode-only changes
If we don't have a new file, deleted file or renamed file in a diff,
we currently add 'patch.new_name' to the range-diff header. This
works well for files that are changed. However if we have a pure mode
change, 'patch.new_na
